[PATCH] D80514: [clang-tidy] modernize-use-trailing-return-type support for C++20 concepts and decltype

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

I've commit on your behalf in 345053390ac17dd4a2e759de9e0e24c2605035db <https://reviews.llvm.org/rG345053390ac17dd4a2e759de9e0e24c2605035db>, thank you for the patch!



================
Comment at: clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:430
+      AT->getKeyword() == AutoTypeKeyword::Auto &&
+      !hasAnyNestedLocalQualifiers(F->getDeclaredReturnType()))
+    return;
----------------
bernhardmgruber wrote:
> aaron.ballman wrote:
> > 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.
> > 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)
> 
> I am sorry to hear that, but I have heard this from many other people as well. I am sometimes questioning myself whether it was a mistake to put this check into clang-tidy and annoy a lot of people. It might have been better as a standalone tool.
> 
> > 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.
> 
> I like that thinking! I started with trailing return types as a stylistic choice, but I soon realized that it indeed can add clarity by shifting away complicated return types to the end of a line where they bother me less.
> 
> > 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).
> 
> With regard to clarity, you are right. And I noted down now that I shall add an option to prevent some of these rewrites. Thank you for the feedback!
> 
> I am sorry to hear that, but I have heard this from many other people as well. I am sometimes questioning myself whether it was a mistake to put this check into clang-tidy and annoy a lot of people. It might have been better as a standalone tool.

FWIW, I don't question whether it was a mistake to add it. I just figure it wasn't targeted at me, and that's fine too. I can see the utility for people who want that kind of declaration style.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80514/new/

https://reviews.llvm.org/D80514