[PATCH] D53025: [clang-tidy] implement new check for const return types.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:93
+        Diagnostics << FixItHint::CreateRemoval(
+            CharSourceRange::getTokenRange(*PrevLoc, *PrevLoc));
+      } else {
----------------
JonasToth wrote:
> ymandel wrote:
> > JonasToth wrote:
> > > Twice `*PrevLoc`?
> > Is there a better alternative? I thought that, since token ranges closed on both ends,  this constructs a SourceRange that spans the single token at PrevLoc.  Instead, I could rework `getConstTokLocations` to return the actual tokens instead, and then create CharSourceRanges from Tok.getLocation(), Tok.getLastLocation().
> The Fixits work with sourcelocations, you dont need to get the tokenrange and CharSourceRange things. This looks more complicated then necessary to me. Your `findConstToRemove` can return a `Optional<SourceRange>` that encloses the `const` token. This range can then be used for the FixIts. I think this would make the code a bit clearer as well.
After reworking to pipe the Token all the way through to the use, I ended up sticking with CharSourceRange::getCharRange(), because otherwise the fixithint will convert a plain SourceRange into a token range, which is not what we want (since we already have the lower-level char range from the lexed token).


================
Comment at: clang-tidy/utils/LexerUtils.cpp:41
+  const char *TokenBegin = File.data() + LocInfo.second;
+  Lexer RawLexer(SM.getLocForStartOfFile(LocInfo.first), Context.getLangOpts(),
+                 File.begin(), TokenBegin, File.end());
----------------
JonasToth wrote:
> I think this function can be simplified as the manual lexing stuff seems unnecessary.
> Take a look at https://reviews.llvm.org/D51949#change-B_4XlTym3KPw that uses `clang::Lexer` functionality quite a bit to do manual lexing.
> 
> You can use the public static methods from `clang::Lexer` to simplify this function.
(FWIW, this code was taken (almost directly) from clang-tidy/readability/AvoidConstParamsInDecls.cpp.)

I agree that would potentially simplify the code here, but I'm not sure its the right thing to do. Basically all of the relevant functions create a clang::Lexer internally and do a bunch more work.  So, it seems like this case, where we are incrementally proceeding through a source, token by token, we really should be using a (stateful) clang::Lexer directly, rather than repeatedly calling into the static methods and creating/destroying a Lexer with each such call.




Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53025