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