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

Sat May 4 08:28:34 PDT 2019

bernhardmgruber added inline comments.

================
Comment at: clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:274
+
+  if (F->getLocation().isInvalid())
+    return;
----------------
aaron.ballman wrote:
> 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?
> How about we not add any options and if someone files a bug report, we can address it then?

Sounds good to me!

================
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 {};
+      }
----------------
aaron.ballman wrote:
> 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.
```
#define const const
const int f(void); // Why no fixit?
```
This can be handled now. As well as e.g.:

```
#define ATT __attribute__((deprecated))
ATT const int f();
```

================
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
----------------
aaron.ballman wrote:
> 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.
Specifiers at arbitrary locations inside the return type should be supported now.

================
Comment at: clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:268
+  const BuiltinType *BT = dyn_cast<BuiltinType>(RT.getTypePtr());
+  if (!BT || !BT->getName(PrintingPolicy(LangOpts)).contains(' '))
+    return true;
----------------
bernhardmgruber wrote:
> I am not sure if this covers all types where a specifier may occur "inside" a type. Can someone come up with something other than a built-in type consisting of at least two tokens?
`int static&();`
Running `keepSpecifiers()` on all return types now.

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

https://reviews.llvm.org/D56160