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