ymandel 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(
----------------
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.
================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:79
+ Decl = Decl->getPreviousDecl()) {
+ if (const llvm::Optional<SourceLocation> PrevLoc =
+ findConstToRemove(Decl, Result)) {
----------------
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.
================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:93
+ Diagnostics << FixItHint::CreateRemoval(
+ CharSourceRange::getTokenRange(*PrevLoc, *PrevLoc));
+ } else {
----------------
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().
================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:95
+ } else {
+ UnfixabledDecls.emplace_back(
+ Decl->getInnerLocStart(),
----------------
JonasToth wrote:
> Did you consider `diag(Loc, "could not transform this declaration",
> DiagnosticIDs::Note)` which emits a `note` instead of `warning`.
> You dont need to store these in between.
Yes, not sure what I was thinking there (storing the same string repeatedly).
Done, thanks.
================
Comment at: test/clang-tidy/Inputs/readability-const-value-return/macro-def.h:1
+#ifndef MACRO_DEF_H
+#define MACRO_DEF_H
----------------
JonasToth wrote:
> ymandel wrote:
> > 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.
> This test is not necessary. If that would not work, the unit tests for the
> frontend need to fail. The inclusions and everything are assumed to be
> correctly done. clang-tidy itself only works on the translation units
> (meaning the .cpp file and all included headers in one blob). You can remove
> this test safely.
Sure. Will do. Just to explain a bit: my concern was whether my code properly
recognizes that the source location at the macro use comes from a macro and I
thought that the cross-file case might be reflected differently in source locs
than the in-file case. But, now that I think about it, I can't see any
interesting difference.
================
Comment at: test/clang-tidy/readability-const-value-return.cpp:53
+ const Strukt* const p7();
+ // CHECK-FIXES: const Strukt* p7();
+
----------------
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?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53025
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
