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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
