aaron.ballman added inline comments.
================
Comment at: clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:274
+
+ if (F->getLocation().isInvalid())
+ return;
----------------
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?
================
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 {};
+ }
----------------
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.
================
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
----------------
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.
================
Comment at: clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:359-364
+ if (!FTL) {
+ // This may happen if we have __attribute__((...)) on the LHS of the
+ // function.
+ diag(F->getLocation(), Message);
+ return;
+ }
----------------
This is suspicious and smells like a bug, to me. I can think of no reason why
this function would have no type location information just because there's a
trailing attribute (I assume you mean RHS here based on the example in your
comment above). It may not be your bug to fix, but knowing whether this works
around a bug or not would be helpful. If it is a bug, the comment should say
something like "FIXME: remove when bug blah is fixed".
================
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;{{$}}
----------------
bernhardmgruber wrote:
> 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?
I think I'm leaning towards consistently diagnosing (and attempting to fix when
possible) everything and we can add options if users ask for them. I was mostly
surprised because I'm used to seeing things in `extern "C"` blocks being C-ish,
but that is personal preference more than anything technical.
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
