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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:25
+
+// Finds the location of the relevant "const" token in `Def`.
+llvm::Optional<SourceLocation> findConstToRemove(
----------------
ymandel wrote:
> JonasToth wrote:
> > ymandel wrote:
> > > JonasToth wrote:
> > > > s/"const"/`const`
> > > here and throughout.  All comments mention const without quotes.
> > I meant that you use the  \` thingie to mark const :)
> > Its better to mark language constructs in the comments as well for better clarity whats meant. Comments need to be full sentences and super clear, otherwise the comment does more harm then helping :)
> > I meant that you use the \` thingie to mark const :)
> Ah. :)  Done now for real.
> 
> > Its better to mark language constructs in the comments as well for better clarity whats meant. Comments need to be full sentences and super clear, otherwise the comment does more harm then helping :)
> 
> Is there a specific issue w/ this comment, or are you remarking in general?  I've rewritten with the goal or better clarity, but wasn't sure of your intent wrt full sentences.  
> 
> 
The comment is ok with a nit: `... source like when the` (drop the comma)


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:64
+
+  // Fix the definition.
+  llvm::Optional<SourceLocation> Loc = findConstToRemove(Def, Result);
----------------
ymandel wrote:
> JonasToth wrote:
> > I feel that this comment doesn't add value. Could you please either make it more expressive or remove it?
> Agreed. I merged the comment below into this one, so one comment describes the rest of the control flow in this block.
Did I tell you the "only one diagnostic in-flight" thing? :D
I told you wrong stuff as you already figured out in the code. Please adjust this comment and the additional scope is not necessary too.


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:79
+       Decl = Decl->getPreviousDecl()) {
+    if (const llvm::Optional<SourceLocation> PrevLoc =
+            findConstToRemove(Decl, Result)) {
----------------
ymandel wrote:
> JonasToth wrote:
> > ymandel wrote:
> > > JonasToth wrote:
> > > > The `const` is not common in llvm-code. Please use it only for references and pointers.
> > > > 
> > > > What do you think about emitting a `note` if this transformation can not be done? It is relevant for the user as he might need to do manual fix-up. It would complicate the code as there can only be one(?) diagnostic at the same time (90% sure only tbh).
> > > Not the most elegant, but I don't see any other way to display multiple diagnoses. Let me know what you think.
> > What do you want to achieve? I think you just want to append the `FixItHint` do you?
> > 
> > you can do this with saving the diagnostic object in a variable.
> > 
> > ```
> > auto Diag = diag(Loc, "Warning Message");
> > 
> > // Foo
> > 
> > if (HaveFix)
> >     Diag << FixItHint::CreateRemove();
> > ```
> > 
> > When `Diag` goes out of scope the diagnostic is actually issued, calling just `diag` produces a temporary that gets immediately destroyed. 
> I was responding to your original suggestion to emit a `note` if the transformation can not be done; I changed the code to allow multiple diagnostics via scoping, but found it less than elegant.
Ok. That multiple diagnostics thing is incorrect, please pretend it was never written ;)


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:93
+        Diagnostics << FixItHint::CreateRemoval(
+            CharSourceRange::getTokenRange(*PrevLoc, *PrevLoc));
+      } else {
----------------
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.


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:90
+      } else {
+        UnfixabledDeclLocs.push_back(Decl->getInnerLocStart());
+      }
----------------
You dont even need to store the location, you can just emit the `diag` here :)


================
Comment at: clang-tidy/utils/LexerUtils.cpp:34
 
+std::vector<SourceLocation> getConstTokLocations(
+    CharSourceRange Range, const clang::ASTContext &Context,
----------------
You can transform this into `std::vector<SourceRange>` to group the ranges, `Token` knows where it ends as well (`Token.getEndLoc()`)


================
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());
----------------
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.


================
Comment at: test/clang-tidy/readability-const-value-return.cpp:53
+  const Strukt* const p7();
+  // CHECK-FIXES: const Strukt* p7();
+
----------------
ymandel wrote:
> JonasToth wrote:
> > ymandel wrote:
> > > JonasToth wrote:
> > > > Missing warning?
> > > No, but this is subtle, so added a comment.
> > I was searching for it, but i overlooked the definition obviously! Do you have a test-case where the definition in not part of the translation unit?
> > 
> > One could split the implementation of a class into mutliple .cpp files and then link them together.
> > For such a case it might be reasonable to not emit the warning for the declaration as there needs to be a definition in the project somewhere. And not emitting the warning removes noise from third party libraries that are just used where only the headers are included.
> Yes, `n14`, below (https://reviews.llvm.org/D53025?id=169290 line 183). I also added a comment there explaining its purpose.
> 
> Sorry, I don't follow.  Currently, we *don't* warn on declarations unless a) the definition is in the TU and b) something gets in the way of removing the `const`.  So, in the situation you're describing (multiple .cpp files), the declaration will be ignored.  Are you explaining why you *agree* with current behavior or are you suggesting a change?
I agree with the current implementation! After reading it again: it was writing while thinking about the issue ;)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53025