[PATCH] D56160: [clang-tidy] modernize-use-trailing-return check

[Bernhard Manfred Gruber via Phabricator via cfe-commits]

bernhardmgruber added a comment.

Thank you @JonasToth for providing more feedback! I will add a few more tests with templates. Maybe I should even try to run the check on Boost and see what happens.

In the meantime I might need some help: I tried running the check on LLVM last weekend using the run-clang-tidy.py file. The script eventually crashed with a segmentation fault (after 1.5 days, running on CentOS 7, 30GiB RAM) with no modifications of the source tree. I ran again and exported the fixes, but again, python failed to merge the yaml files and crashed (probably because it went out of memory). After manual merging, I ran clang-apply-replacements and it took a while, but then I also had zero modifications on my LLVM working copy. clang-apply-replacements reported a few overlapping refactorings and missing files, but that's it. What am I doing wrong?

And btw, is there an easy way to get a compilation database on Windows?

Many thanks!



================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:118
+      if (Info.hasMacroDefinition()) {
+        diag(F.getLocation(), Message); // CV qualifiers in macros
+        return {};
----------------
JonasToth wrote:
> Please improve that comment and here I would prefer a non-trailing comment, too.
> Especially formulate whats with CV and macros, the meaning has to be guessed (and can be guessed wrong).
replaced by "The CV qualifiers of the return type are inside macros"


================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:133
+  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
----------------
JonasToth wrote:
> please use uppercase `i` and `j` names for consistency.
i considered this with respect to the style guide, but it just looked far to unfamiliar to me. done.


================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:162
+      functionDecl(unless(anyOf(hasTrailingReturn(), returns(voidType()),
+                                returns(autoType()), cxxConversionDecl(),
+                                cxxMethodDecl(isImplicit()))))
----------------
JonasToth wrote:
> Shouldn't you include `returns(decltypeType())` as well?
good question! i have a unit test of the form `decltype(auto) f();` and it seems to be already excluded by `returns(autoType())`. but i could add your suggestion as well to make it more explicit that (for now) we will not rewrite functions returning `decltype(auto)`.


================
Comment at: test/clang-tidy/modernize-use-trailing-return.cpp:269
+auto l1 = [](int arg) {};
+auto l2 = [](int arg) -> double {};
----------------
JonasToth wrote:
> These tests are missing the great template fun :)
> 
> Lets start with those two examples:
> 
> ```
> template <typename Container>
> [[maybe_unused]] typename Container::value_type const volatile&& myFunnyFunction(Container& C) noexcept;
> ```
> 
> and
> 
> ```
> #define MAYBE_UNUSED_MACRO [[maybe_unused]]
> template <typename Container>
> MAYBE_UNUSED_MACRO typename Container::value_type const volatile** const myFunnyFunction(Container& C) noexcept;
> ```
> 
> Its not necessarily nice code, but I am sure something like this is somewhere in boost for example ;)
You remind me of Jason Turner at CppCon 2018 who said, we should pick a toy project, for which we are sure we can handle it, because complexity will manifest itself in the details. This is exactly what is happening now.

Thank you for input, I added it to my tests!


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

https://reviews.llvm.org/D56160