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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
