-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
base: develop
Are you sure you want to change the base?
Conversation
@@ -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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :-).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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 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 Bad, but vice versa: std::function<void(int)> f = []() {return 6;}; implicit conversion with performance implications if 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. |
This pull request is stale because it has been open for 14 days with no activity. |
This pull request was closed due to a lack of activity for 7 days after it was stale. |
This pull request is stale because it has been open for 14 days with no activity. |
This pull request is stale because it has been open for 14 days with no activity. |
This pull request is stale because it has been open for 14 days with no activity. |
This pull request is stale because it has been open for 14 days with no activity. |
This pull request is stale because it has been open for 14 days with no activity. |
This pull request is stale because it has been open for 14 days with no activity. |
Just that we have something to argue about :-).