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