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