[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:180-184
+      if (Info.hasMacroDefinition()) {
+        // The CV qualifiers of the return type are inside macros.
+        diag(F.getLocation(), Message);
+        return {};
+      }
----------------
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?
```


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


================
Comment at: clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:293
+  if (!TSI) {
+    diag(F->getLocation(), Message);
+    return;
----------------
This seems incorrect to me; if we cannot get the type source information, why do we assume it has a return type that needs to be turned into a trailing return type?


================
Comment at: clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:298
+      TSI->getTypeLoc().IgnoreParens().getAs<FunctionTypeLoc>();
+  if (!FTL) {
+    diag(F->getLocation(), Message);
----------------
I think this can turn into an assertion that `FTL` is valid.


================
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()`?
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?


================
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.
This comment is marked as done, but there are no changes or explanations.


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

https://reviews.llvm.org/D56160