aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
In D80514#2206019 <https://reviews.llvm.org/D80514#2206019>, @bernhardmgruber wrote: > Btw: what is the general rule for Phabricator reviews here when to tick the > `Done` checkbox of a comment? What is the semantic of this checkbox? Is the > ticked state the same for everyone? I don't think we have any hard-and-fast rules for it and I suspect the ticked state differs from review to review. Personally, I appreciate when the person who was asked to do some work is the one to check the Done box signalling that they think it's done (whether it's an author fixing a comment from a reviewer or a reviewer answering a question from an author, etc). This tells me "hey, someone thinks this is done, go look at it and comment if you disagree." However, I am guessing others have a different workflow. I don't think I have any further concerns with this patch, so LGTM. ================ Comment at: clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:430 + AT->getKeyword() == AutoTypeKeyword::Auto && + !hasAnyNestedLocalQualifiers(F->getDeclaredReturnType())) + return; ---------------- bernhardmgruber wrote: > aaron.ballman wrote: > > aaron.ballman wrote: > > > bernhardmgruber wrote: > > > > aaron.ballman wrote: > > > > > Why do we need to check that there aren't any nested local qualifiers? > > > > Because I would like the check to rewrite e.g. `const auto f();` into > > > > `auto f() -> const auto;`. If I omit the check for nested local > > > > qualifiers, then those kind of declarations would be skipped. > > > I'm still wondering about this. > > > Because I would like the check to rewrite e.g. const auto f(); into auto > > > f() -> const auto;. If I omit the check for nested local qualifiers, then > > > those kind of declarations would be skipped. > > > > I don't think I understand why that's desirable though? What is it about > > the qualifier that makes it worthwhile to repeat the type like that? > Thank you for insisting on that peculiarity! The choice is stylistically > motivated to align function names: > > ``` > auto f() -> int; > auto g() -> std::vector<float>; > auto& h(); > const auto k(); > decltype(auto) l(); > ``` > vs. > ``` > auto f() -> int; > auto g() -> std::vector<float>; > auto h() -> auto&; > auto k() -> const auto; > auto l() -> decltype(auto); > ``` > > But judging from your response, this might be a surprise to users. Would you > prefer having an option to enable/disable this behavior? > But judging from your response, this might be a surprise to users. Would you > prefer having an option to enable/disable this behavior? Maybe it will be, maybe it won't. :-D The reason I was surprised was because it feels like a formatting related choice rather than a modernization related choice. However, I've always struggled to understand the utility of this check (it's one I disable in every .clang-tidy configuration file I can), so my reasoning may be flawed. I feel like the goal of this check isn't to format code nicely, it's to modernize to using a trailing return type where that adds clarity. But I don't think `auto& func()` changing into `auto func() -> auto&` adds clarity (I think it removes clarity because the second signature is strictly more complex than the first), and similar for qualifiers. However, I think the exact same thing about `int func()` changing into `auto func() -> int`. Given that we document this function to rewrite all functions to a trailing return type signature, I guess the behavior you've proposed here is consistent with that goal and so I'm fine with it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80514/new/ https://reviews.llvm.org/D80514 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
