[PATCH] D56160: [clang-tidy] modernize-use-trailing-return-type check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:274
+
+  if (F->getLocation().isInvalid())
+    return;
----------------
bernhardmgruber wrote:
> aaron.ballman wrote:
> > bernhardmgruber wrote:
> > > aaron.ballman wrote:
> > > > Should this also bail out if the function is `main()`?
> > > How strange does
> > > 
> > > ```
> > > auto main(int argc, const char* argv[]) -> int {
> > >     return 0;
> > > }
> > > ```
> > > look to you?
> > > 
> > > I think the transformation is valid here. But I can understand that people feel uneasy after typing `int main ...` for decades. Should we also create an option here to turn it off for main? Or just not transform it? I do not mind. If I want `main` to start with `auto` I could also do this transformation manually.
> > This comment was marked as done, but I don't see any changes or mention of what should happen. I suppose the more general question is: should there be a list of functions that should not have this transformation applied, like program entrypoints? Or do you want to see this check diagnose those functions as well?
> I am sorry for marking it as done. I do not know how people work here exactly and how phabricator behaves. I thought the check boxes are handled for everyone individually. Also, if I add a new comment, it is checked by default.
> 
> How are you/most people going to use clang-tidy? Do you run it regularly and automatic? Do you expect it to find zero issues once you applied the check?
> In other words, if you do not want to transform some functions, do you need an option to disable the check for these, so it runs clean on the full source code?
> 
> Personally, I do not need this behavior, as I run clang-tidy manually once in a while and revert transformations I do not want before checking in the changes.
> I am sorry for marking it as done. I do not know how people work here exactly and how phabricator behaves. I thought the check boxes are handled for everyone individually. Also, if I add a new comment, it is checked by default.

No worries -- that new comments are automatically marked done by default is certainly a surprise to me!

> How are you/most people going to use clang-tidy? Do you run it regularly and automatic? Do you expect it to find zero issues once you applied the check? In other words, if you do not want to transform some functions, do you need an option to disable the check for these, so it runs clean on the full source code?

I think it's hard to gauge how most people do anything, really. However, I think there are people who enable certain clang-tidy checks and have them run automatically as part of CI, etc. Those kind of folks may need the ability to silence the diagnostics. We could do this in a few ways (options to control methods not to diagnose, NOLINT comments, etc).

I kind of think we don't need an option for the user to list functions not to transform. They can use NOLINT to cover those situations as a one-off. The only situation where I wonder if everyone is going to want to write NOLINT is for `main()`. It might make sense to have an option to not check program entrypoints, but there is technically nothing stopping someone from using a trailing return type with a program entrypoint so maybe this option isn't needed at all.

How about we not add any options and if someone files a bug report, we can address it then?


================
Comment at: clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:180-184
+      if (Info.hasMacroDefinition()) {
+        // The CV qualifiers of the return type are inside macros.
+        diag(F.getLocation(), Message);
+        return {};
+      }
----------------
bernhardmgruber wrote:
> aaron.ballman wrote:
> > Perhaps I'm a bit dense on a Monday morning, but why should this be a diagnostic? I am worried this will diagnose situations like (untested guesses):
> > ```
> > #define const const
> > const int f(void); // Why no fixit?
> > 
> > #define attr __attribute__((frobble))
> > attr void g(void); // Why diagnosed as needing a trailing return type?
> > ```
> Because I would also like to rewrite functions which contain macros in the return type. However, I cannot provide a fixit in all cases. Clang can give me a `SourceRange` without CV qualifiers which seems to work in all my tests so far. But to include CV qualifiers I have to do some manual lexing left and right of the return type `SourceRange`. If I encounter macros along this way, I bail out because handling these cases becomes compilated very easily.
> 
> Your second case does not give a diagnostic, as it is not matched by the check, because it returns `void`.
> Because I would also like to rewrite functions which contain macros in the return type. However, I cannot provide a fixit in all cases. Clang can give me a SourceRange without CV qualifiers which seems to work in all my tests so far. But to include CV qualifiers I have to do some manual lexing left and right of the return type SourceRange. If I encounter macros along this way, I bail out because handling these cases becomes compilated very easily.

That makes sense, but I am worried about this bailing out because of things that are not CV qualifiers but are typically macros, like attributes. It seems like there should not be a problem providing a fixit in that situation, unless I'm misunderstanding still.


================
Comment at: clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:234
+  bool ExtendedLeft = false;
+  for (size_t I = 0; I < Tokens.size(); I++) {
+    // If we found the beginning of the return type, include const and volatile
----------------
bernhardmgruber wrote:
> aaron.ballman wrote:
> > As a torture test for this, how well does it handle a declaration like:
> > ```
> > const long static int volatile unsigned inline long foo();
> > ```
> > Does it get the fixit to spit out:
> > ```
> > static inline auto foo() -> const unsigned long long int;
> > ```
> I honestly did not believe this compiled until I put it into godbolt. And no, it is not handled.
> I added your test as well as a few other ones of this kind. You could also add `constexpr` or inside classes `friend` anywhere.
> 
> I will try to come up with a solution. I guess the best would be to delete the specifiers from the extracted range type string and readd them in the order of appearance before auto.
> I will try to come up with a solution. I guess the best would be to delete the specifiers from the extracted range type string and readd them in the order of appearance before auto.

Alternatively, if it's easier, you can refuse to add fix-its for the situation and just add a FIXME to handle this should users ever care.


================
Comment at: clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:359-364
+  if (!FTL) {
+    // This may happen if we have __attribute__((...)) on the LHS of the
+    // function.
+    diag(F->getLocation(), Message);
+    return;
+  }
----------------
This is suspicious and smells like a bug, to me. I can think of no reason why this function would have no type location information just because there's a trailing attribute (I assume you mean RHS here based on the example in your comment above). It may not be your bug to fix, but knowing whether this works around a bug or not would be helpful. If it is a bug, the comment should say something like "FIXME: remove when bug blah is fixed".


================
Comment at: test/clang-tidy/modernize-use-trailing-return-type.cpp:95-97
+extern "C" int d2(int arg);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: use a trailing return type for this function [modernize-use-trailing-return-type]
+// CHECK-FIXES: {{^}}extern "C" auto d2(int arg) -> int;{{$}}
----------------
bernhardmgruber wrote:
> aaron.ballman wrote:
> > bernhardmgruber wrote:
> > > aaron.ballman wrote:
> > > > This is a rather unexpected transformation, to me.
> > > It felt a bit strange initially, but as it is a valid transformation, i have included it. Do you think we should create an option to turn this off?
> > This comment is marked as done, but there are no changes or explanations.
> I am sorry.
> 
> What do you think about the transformation? Should there be an option to disable transforming `extern "C"` statements?
I think I'm leaning towards consistently diagnosing (and attempting to fix when possible) everything and we can add options if users ask for them. I was mostly surprised because I'm used to seeing things in `extern "C"` blocks being C-ish, but that is personal preference more than anything technical.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56160/new/

https://reviews.llvm.org/D56160