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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> It took me far too long to come up with an update. Honestly, I was quite demotivated as it turned out preventing name collisions of unqualifed names after the rewrite was more difficult than I thought. Especially because this error occurs sparsely when I test the check on bigger code bases. The current approach with the AST visitor seems to work best and maybe I can even extend it at some point to automatically qualify colliding names. But for now, I would be really glad if we could achieve a state of the check that you guys can accept and commit.

I totally know that feeling! If there is a wrong transformation it would always be catched on compilation, right? I am fine with a check that handles 99% of code correct and we do have other checks that have similar limitations. If you find a way to exclude the wrong cases from transformation even better.



================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:78
+  // TraverseTypeLoc().
+  // TODO: Maybe RecursiveASTVisitor should call
+  // getDerived().TraverseTypeLoc(...).
----------------
bernhardmgruber wrote:
> TODO: is this an issue in RecursiveASTVisitor or is this behavior intended? Everywhere else, it calls `getDerived().XXX();`
I don't know, but its not a big issue, is it?


================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:335
+  StringRef ReturnType = tooling::fixit::getText(ReturnTypeCVRange, Ctx);
+  StringRef Auto = std::isspace(*ReturnType.end()) // FIXME (dereferencing end)
+                       ? "auto"
----------------
bernhardmgruber wrote:
> FIXME: this feels wrong when I wrote it, but it works. I tried to fiddle with the ReturnTypeCVRange to include the next charakter, but I could not get it working without writing `tooling::fixit::getText` myself. Any ideas?
That only works, because `StringRef` points within the orignal code buffer. IMHO just adding the space always and letting clang-format do its job is better then this potential out-of-bounds read.
Maybe you could increase the `ReturnTypeCVRange` by one at the end, then its fine.


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

https://reviews.llvm.org/D56160