ymandel added a comment.
Thank you for the detailed comments! They were quite helpful, especially as
this is my first llvm/clang review.
================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:25
+
+// Finds the location of the relevant "const" token in `Def`.
+llvm::Optional<SourceLocation> findConstToRemove(
----------------
JonasToth wrote:
> s/"const"/`const`
here and throughout. All comments mention const without quotes.
================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:60
+ const auto *Def = Result.Nodes.getNodeAs<FunctionDecl>("func");
+ if (!Def->getReturnType().isLocalConstQualified()) {
+ return;
----------------
JonasToth wrote:
> Please ellide the braces
Actually, i expanded this to include a diag() statement, so kept the braces.
================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:64
+
+ // Fix the definition.
+ llvm::Optional<SourceLocation> Loc = findConstToRemove(Def, Result);
----------------
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.
================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:65
+ // Fix the definition.
+ llvm::Optional<SourceLocation> Loc = findConstToRemove(Def, Result);
+ if (!Loc)
----------------
JonasToth wrote:
> 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?
Nice. I like this.
================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:79
+ Decl = Decl->getPreviousDecl()) {
+ if (const llvm::Optional<SourceLocation> PrevLoc =
+ findConstToRemove(Decl, Result)) {
----------------
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.
================
Comment at: docs/clang-tidy/checks/list.rst:12
abseil-redundant-strcat-calls
- abseil-string-find-startswith
abseil-str-cat-append
----------------
JonasToth wrote:
> spurious change
right. this was done by the script, so I wonder why it reordered these.
================
Comment at: test/clang-tidy/Inputs/readability-const-value-return/macro-def.h:1
+#ifndef MACRO_DEF_H
+#define MACRO_DEF_H
----------------
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.
================
Comment at: test/clang-tidy/readability-const-value-return.cpp:53
+ const Strukt* const p7();
+ // CHECK-FIXES: const Strukt* p7();
+
----------------
JonasToth wrote:
> Missing warning?
No, but this is subtle, so added a comment.
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
