[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:
> > 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 :)


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


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:57
+  if (!Def->getReturnType().isLocalConstQualified()) {
+    // From the matcher, we know this is const qualified.  But it is not done
+    // locally, so we can't offer a fix -- just a warning.
----------------
Please make the second sentence a gramatically correct english sentence, right now it's a little off.


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:60
+    diag(Def->getInnerLocStart(),
+         "return type is 'const'-qualifed (possibly behind a type alias), "
+         "which hinders compiler optimizations");
----------------
This diagnostic is nice, but including the type would make it even better. This will resolve typedefs as well (like "'uint32_t (a.k.a. 'unsigned int')").

Necessary code (more info in the cfe-internals manual)
```
diag(Location, "return type %0 is 'const'-qualified hindering compiler optimizations") << Def->getReturnType();
```
The value passed in with `<<` must be a `QualType`.
Also diagnostics don't have punctuation and are not full sentences and should be as short as possible (but still clear).


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:69
+    // a macro. So, warn the user, but offer no fix.
+    diag(Def->getInnerLocStart(),
+         "return type is 'const'-qualifed (possibly behind a macro), which "
----------------
This duplicates the diagnostic above. The diagnostics engine would resolve macro expansion and emit the `note: expanded from XXX`. I think this does not add value and introduces duplication.


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:83
+    // warning at the definition.
+    DiagnosticBuilder Diagnostics =
+        diag(*Loc,
----------------
You don't need this diagnostic again, you can just use the one and first diagnostic from the beginning. Right now you are creating to many warnings which are all redundant.
Please refactor the logic to

1. Create warning for everything because the matcher found an offending definition
2. If(!NotFixable) return
3. CreateFixOtherwiseForAllDefintions

This looks simpler to me.


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:93
+        Diagnostics << FixItHint::CreateRemoval(
+            CharSourceRange::getTokenRange(*PrevLoc, *PrevLoc));
+      } else {
----------------
Twice `*PrevLoc`?


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:95
+      } else {
+        UnfixabledDecls.emplace_back(
+            Decl->getInnerLocStart(),
----------------
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.


================
Comment at: docs/clang-tidy/checks/list.rst:12
    abseil-redundant-strcat-calls
-   abseil-string-find-startswith
    abseil-str-cat-append
----------------
ymandel wrote:
> JonasToth wrote:
> > spurious change
> right. this was done by the script, so I wonder why it reordered these.
It reads all checks, orders and inserts the new one and prints them out again, thats why the change happened.


================
Comment at: test/clang-tidy/Inputs/readability-const-value-return/macro-def.h:1
+#ifndef MACRO_DEF_H
+#define MACRO_DEF_H
----------------
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.


================
Comment at: test/clang-tidy/check_clang_tidy.py:110
 
-      has_check_fix = check_fixes_prefix in input_text 
       has_check_message = check_messages_prefix in input_text
----------------
please remove these changes once the fix for that is committed upstream.


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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53025