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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I believe there is some sort of memory leak in the `run-clang-tidy` or so. I had similar experience :)
Take this with a grain of salt, as I don't recall all details: `run-clang-tidy.py` just takes all output from clang-tidy and prints it. A lot is redundant due to the include-horror in c++. After I implemented deduplication (not in tree, but here https://reviews.llvm.org/D54141) things got better. You don't need to run it over the whole of LLVM, but can take a subset, too. Maybe `lib/` or `tools/clang/lib`. Note that `run-clang-tidy.py` reads in the compilation database and matches your path as regex against it. So `lib/` includes all libs (clang, llvm, ...).

> 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?

Overlapping can not be resolved, but interesting it occurs. Maybe in headers? Each TU (~ .cpp-file) will emit transformations. If they are identical they are merged, but if not there is a collision which can not be resolved. Maybe there is something going on with macros, ... that make the same header included twice not equivalent or so?

Try to run it over a subset first and then think about the issues. Trying other projects is certainly a good idea as well.

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

No windows-user, but cmake creates one with `CMAKE_EXPORT_COMPILECOMMANDS=ON` that you need to pass in somewhere somehow :)



================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:162
+      functionDecl(unless(anyOf(hasTrailingReturn(), returns(voidType()),
+                                returns(autoType()), cxxConversionDecl(),
+                                cxxMethodDecl(isImplicit()))))
----------------
bernhardmgruber wrote:
> 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)`.
There should be a difference between `decltype(auto)` and `decltype(some_expression)`. I will check your tests and suggest something there.


================
Comment at: test/clang-tidy/modernize-use-trailing-return.cpp:269
+auto l1 = [](int arg) {};
+auto l2 = [](int arg) -> double {};
----------------
bernhardmgruber wrote:
> 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!
Templates made me sad often, after i "finished" my checks and ran them over projects ;)
Sometimes it is almost impossible to fix very weird (and uncommon) things. But its better to find such cases early and work around them.


================
Comment at: test/clang-tidy/modernize-use-trailing-return.cpp:116
+
+// TODO: not matched by the AST matcher
+//decltype(auto) e6();
----------------
Is this still the case? That should be doable with the matchers, if you have a specific question i can look at it and try to help.

Here would be a good place to test the different `decltype`s.

```
decltype(auto) foo0() { return 1 + 2; } // should be the same as ...
auto foo1() { return 1 + 2; } // this, BUT ...
decltype(1 + 2) foo2() { return 1 + 2; } // should be something else
```
The matching for all three of them should be done differently.


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

https://reviews.llvm.org/D56160