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

[Bernhard Manfred Gruber via Phabricator via cfe-commits]

bernhardmgruber added a comment.

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?

Thank you for the help!



================
Comment at: clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:430
+      AT->getKeyword() == AutoTypeKeyword::Auto &&
+      !hasAnyNestedLocalQualifiers(F->getDeclaredReturnType()))
+    return;
----------------
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?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type.cpp:1
-// RUN: %check_clang_tidy -std=c++14,c++17 %s modernize-use-trailing-return-type %t -- -- -fdeclspec -fexceptions
+// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-use-trailing-return-type %t -- -- -fdeclspec -fexceptions -DCOMMAND_LINE_INT=int
 // FIXME: Fix the checker to work in C++20 mode, it is performing a
----------------
riccibruno wrote:
> riccibruno wrote:
> > aaron.ballman wrote:
> > > bernhardmgruber wrote:
> > > > aaron.ballman wrote:
> > > > > The change to the language standard line makes me wonder if the fixme below it is now stale, or if the test will fail in C++20 mode.
> > > > I just ran the tests again using `python .\check_clang_tidy.py -std=c++20 .\checkers\modernize-use-trailing-return-type.cpp modernize-use-trailing-return-type aa -- -- -DCOMMAND_LINE_INT=int` and I did not see mentioning of the use of an uninitialized variable. But I run on Windows, maybe the issue just surfaces on another OS? Is there a way to trigger the CI again?
> > > > 
> > > > I removed the FIXME in question in the hope the issue resolved itself.
> > > > But I run on Windows, maybe the issue just surfaces on another OS? Is there a way to trigger the CI again?
> > > 
> > > I also run on Windows so I can't easily test the behavior elsewhere for you. The CI will get triggered on new patch uploads, but I still don't always trust it. The bots are usually a more reliable source of CI truth but we have no way to speculatively trigger all the bots.
> > I can run it for you with asan/msan.
> No warning with either asan or msan on x86-64 linux with clang 10.
Thank you @riccibruno for verifying that for me!


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

https://reviews.llvm.org/D80514