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:
> > 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.
>
>
The comment is ok with a nit: `... source like when the` (drop the comma)
================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:64
+
+ // Fix the definition.
+ llvm::Optional<SourceLocation> Loc = findConstToRemove(Def, Result);
----------------
ymandel wrote:
> 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.
Did I tell you the "only one diagnostic in-flight" thing? :D
I told you wrong stuff as you already figured out in the code. Please adjust
this comment and the additional scope is not necessary too.
================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:79
+ Decl = Decl->getPreviousDecl()) {
+ if (const llvm::Optional<SourceLocation> PrevLoc =
+ findConstToRemove(Decl, Result)) {
----------------
ymandel wrote:
> 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.
Ok. That multiple diagnostics thing is incorrect, please pretend it was never
written ;)
================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:93
+ Diagnostics << FixItHint::CreateRemoval(
+ CharSourceRange::getTokenRange(*PrevLoc, *PrevLoc));
+ } else {
----------------
ymandel wrote:
> 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().
The Fixits work with sourcelocations, you dont need to get the tokenrange and
CharSourceRange things. This looks more complicated then necessary to me. Your
`findConstToRemove` can return a `Optional<SourceRange>` that encloses the
`const` token. This range can then be used for the FixIts. I think this would
make the code a bit clearer as well.
================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:90
+ } else {
+ UnfixabledDeclLocs.push_back(Decl->getInnerLocStart());
+ }
----------------
You dont even need to store the location, you can just emit the `diag` here :)
================
Comment at: clang-tidy/utils/LexerUtils.cpp:34
+std::vector<SourceLocation> getConstTokLocations(
+ CharSourceRange Range, const clang::ASTContext &Context,
----------------
You can transform this into `std::vector<SourceRange>` to group the ranges,
`Token` knows where it ends as well (`Token.getEndLoc()`)
================
Comment at: clang-tidy/utils/LexerUtils.cpp:41
+ const char *TokenBegin = File.data() + LocInfo.second;
+ Lexer RawLexer(SM.getLocForStartOfFile(LocInfo.first), Context.getLangOpts(),
+ File.begin(), TokenBegin, File.end());
----------------
I think this function can be simplified as the manual lexing stuff seems
unnecessary.
Take a look at https://reviews.llvm.org/D51949#change-B_4XlTym3KPw that uses
`clang::Lexer` functionality quite a bit to do manual lexing.
You can use the public static methods from `clang::Lexer` to simplify this
function.
================
Comment at: test/clang-tidy/readability-const-value-return.cpp:53
+ const Strukt* const p7();
+ // CHECK-FIXES: const Strukt* p7();
+
----------------
ymandel wrote:
> 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?
I agree with the current implementation! After reading it again: it was writing
while thinking about the issue ;)
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
