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

[Bernhard Manfred Gruber via Phabricator via cfe-commits]

bernhardmgruber marked an inline comment as not done.
bernhardmgruber added a comment.

Thank you for the rich feedback @aaron.ballman. I found a solution which seems to work for many of my test cases.



================
Comment at: clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:274
+
+  if (F->getLocation().isInvalid())
+    return;
----------------
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.


================
Comment at: clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:274
+
+  if (F->getLocation().isInvalid())
+    return;
----------------
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.


================
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:
> 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`.


================
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:
> 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.


================
Comment at: clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:298
+      TSI->getTypeLoc().IgnoreParens().getAs<FunctionTypeLoc>();
+  if (!FTL) {
+    diag(F->getLocation(), Message);
----------------
aaron.ballman wrote:
> I think this can turn into an assertion that `FTL` is valid.
I actually found a case where this is null: `int unsigned att() __attribute__((cdecl));`


================
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;
----------------
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?


================
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;{{$}}
----------------
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?


================
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;{{$}}
----------------
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?


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

https://reviews.llvm.org/D56160