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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Hi  ymandel,

welcome to the LLVM community and thank you very much for working on that check!
If you have any questions or other issues don't hesitate to ask ;)



================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:25
+
+// Finds the location of the relevant "const" token in `Def`.
+llvm::Optional<SourceLocation> findConstToRemove(
----------------
s/"const"/`const`


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:26
+// Finds the location of the relevant "const" token in `Def`.
+llvm::Optional<SourceLocation> findConstToRemove(
+    const FunctionDecl *Def, const MatchFinder::MatchResult &Result) {
----------------
Please use a `static` function instead of a anonymous namespace. These are only used for local classes.


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:37
+  if (FileRange.isInvalid())
+    return {};
+
----------------
Please return `None` instead, same below


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:60
+  const auto *Def = Result.Nodes.getNodeAs<FunctionDecl>("func");
+  if (!Def->getReturnType().isLocalConstQualified()) {
+    return;
----------------
Please ellide the braces


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:64
+
+  // Fix the definition.
+  llvm::Optional<SourceLocation> Loc = findConstToRemove(Def, Result);
----------------
I feel that this comment doesn't add value. Could you please either make it more expressive or remove it?


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:65
+  // Fix the definition.
+  llvm::Optional<SourceLocation> Loc = findConstToRemove(Def, Result);
+  if (!Loc)
----------------
This check does not produce diagnostics if something in the lexing went wrong.
I think even if its not possible to do transformation the warning could still be emitted (at functionDecl location). What do you think?


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:71
+      diag(*Loc,
+           "avoid marking return types as ``const`` for values, as it often  "
+           "disables optimizations and causes unnecessary copies")
----------------
please shorten that warning a bit and use 'const'.

Maybe `'const'-qualified return values hinder compiler optimizations` or something in this direction?


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


================
Comment at: docs/clang-tidy/checks/list.rst:12
    abseil-redundant-strcat-calls
-   abseil-string-find-startswith
    abseil-str-cat-append
----------------
spurious change


================
Comment at: docs/clang-tidy/checks/readability-const-value-return.rst:19
+Note that this does not apply to returning pointers or references to const
+values.  These are fine:
+
----------------
please remove the double space


================
Comment at: test/clang-tidy/Inputs/readability-const-value-return/macro-def.h:1
+#ifndef MACRO_DEF_H
+#define MACRO_DEF_H
----------------
You can define these macros directly in the test-case, or do you intend something special? Self-contained tests are prefered.


================
Comment at: test/clang-tidy/readability-const-value-return.cpp:4
+
+#include "macro-def.h"
+
----------------
Please add tests, were the `const` is hidden behind typedefs/using. 

I feel that there should be more test that try to break the lexing part as well.
E.g. `const /** I am a comment */ /* weird*/ int function();` be creative here ;)

Could you please add a test-case `int ** const * multiple_ptr();` and `int *const & pointer_ref();`


================
Comment at: test/clang-tidy/readability-const-value-return.cpp:53
+  const Strukt* const p7();
+  // CHECK-FIXES: const Strukt* p7();
+
----------------
Missing warning?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53025