[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:25
+
+// Finds the location of the relevant "const" token in `Def`.
+llvm::Optional<SourceLocation> findConstToRemove(
----------------
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.  




================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:79
+       Decl = Decl->getPreviousDecl()) {
+    if (const llvm::Optional<SourceLocation> PrevLoc =
+            findConstToRemove(Decl, Result)) {
----------------
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.


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


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:95
+      } else {
+        UnfixabledDecls.emplace_back(
+            Decl->getInnerLocStart(),
----------------
JonasToth wrote:
> Did you consider `diag(Loc, "could not transform this declaration", DiagnosticIDs::Note)` which emits a `note` instead of `warning`.
> You dont need to store these in between.
Yes, not sure what I was thinking there (storing the same string repeatedly).  Done, thanks.


================
Comment at: test/clang-tidy/Inputs/readability-const-value-return/macro-def.h:1
+#ifndef MACRO_DEF_H
+#define MACRO_DEF_H
----------------
JonasToth wrote:
> ymandel wrote:
> > JonasToth wrote:
> > > You can define these macros directly in the test-case, or do you intend something special? Self-contained tests are prefered.
> > I added a comment explaining. Does that justify its existence? If not, I'm fine getting rid of this header.
> This test is not necessary. If that would not work, the unit tests for the frontend need to fail. The inclusions and everything are assumed to be correctly done. clang-tidy itself only works on the translation units (meaning the .cpp file and all included headers in one blob). You can remove this test safely.
Sure.  Will do.  Just to explain a bit: my concern was whether my code properly recognizes that the source location at the macro use comes from a macro and I thought that the cross-file case might be reflected differently in source locs than the in-file case.  But, now that I think about it, I can't see any interesting difference.


================
Comment at: test/clang-tidy/readability-const-value-return.cpp:53
+  const Strukt* const p7();
+  // CHECK-FIXES: const Strukt* p7();
+
----------------
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?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53025