Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Saner treatment of auto in coding style. #15077

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

ekpyron
Copy link
Member

@ekpyron ekpyron commented May 6, 2024

Just that we have something to argue about :-).

CODING_STYLE.md Outdated Show resolved Hide resolved
@@ -112,10 +112,9 @@ Use `solAssert` and `solUnimplementedAssert` generously to check assumptions tha
6. If a function returns multiple values, use std::tuple (std::pair acceptable) or better introduce a struct type. Do not use */& arguments.
7. Use parameters of pointer type only if ``nullptr`` is a valid argument, use references otherwise. Often, ``std::optional`` is better suited than a raw pointer.
8. Never use a macro where adequate non-preprocessor C++ can be written.
9. Only use ``auto`` if the type is very long and rather irrelevant.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd not be opposed to relaxing this rule, but I think that just outright removing it is a bad idea. We should still limit the use of auto somewhat.

The thing that I dislike about auto the most is that it optimizes for writing code rather than reading it and for writing it's not even a meaningful speedup because modern completion and IDE features make that already easy. On the other hand it shifts the effort of figuring out the type from the one who writes the code to the one who reads it. Even if the type can be seen from the nearby context, having to fish it out slows down reading. It's only really redundant if the type is obvious directly from the expression it's used on.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wholeheartedly disagree here. Whenever you can't read the code due to an auto, there's something wrong with it other than the auto - it doesn't allow you to skip types, if they are relevant, since you still have type-checking - in all cases in which you can use auto, it's better to use auto, since the code should be apparent as correct despite not knowing the respective type. What slows down reading code is having to wonder about pointer-, reference- or const-ness of things, so those I'd usually preserve despite using auto.
I can bring the point back in the way that's my preferred style for contrast :-).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that's just to make the point :-). You're reviewing more code than I these days, so I'd generally assign more value to your preference here in the end.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually even argue the contrary: not using auto makes code harder to read, since then I actually need to check, if the type is correct and whether there was an implicit conversion or the like, while if I use auto, I do know all relevant aspects of the type, despite it not being written explicitly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, having an example to argue about would probably be helpful. Maybe I should try to come up with one too.

Using wrong type can be bad if it compiles but more often than not it will not. It will instead let compiler catch your bad assumption.

Overall I do think we should relax this but I think what you propose is a bit too extreme :) But I'll come back to this with more concrete arguments after the meeting.

Copy link
Member Author

@ekpyron ekpyron May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, no worries - we can also collect more opinions in the next days :-). And don't need to be too extreme with it. But yeah: to argue the point: the compiler will, in a lot of cases, accept wrong types, and perform implicit conversions, which may not be what you want at all. So I don't buy the readability and neither the safety argument really :-).

Also: official recommendations are on my side: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es11-use-auto-to-avoid-redundant-repetition-of-type-names

@clonker
Copy link
Member

clonker commented May 7, 2024

I am all for the use of auto, as it removes unwanted implicit conversions and (IMO) helps readability if the code is well-written in the first place. :-D

Bad:

auto result = computeSomeStuff(x, y, z);
auto result2 = computeSomeStuff(x, z, y);

here it's not clear what types result and result2 are; also they can be drastically different types to begin with.

Potentially bad:

auto result = Eigen::MatrixXd::Random(3, 3).inverse();

this would return some weird intermediate proxy thing but not actually (yet) compute the inverse. This can be desired but may also lead to unexpected behavior. The actual result is obtained by writing MatrixXd result = .... Although I've never encountered such behavior outside of Eigen (or other linalg libraries) where you'd have lazy computation of individual elements of a tensor.

Bad, but vice versa:

std::function<void(int)> f = []() {return 6;};

implicit conversion with performance implications if f is evaluated frequently.

Of course there are classical examples like iterator types derived from some map or so, which are super unwieldy. I guess there is no discussion that here auto is always preferable.

In the end I feel like it's hard to find a general rule like "always use auto" or "never use auto" as it's depending on context and generally depending on what you're working with. But personally, I would turn the statement around and say something to the effect of: use explicit types sparingly and deliberately if they can be replaced by auto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants