ymandel marked an inline comment as done.
ymandel added a comment.
Thank you for the detailed review. I've significantly expanded and refactored
the tests. I also lifted `validateEditRange` into its own function and added
corresponding tests.
================
Comment at: clang/lib/Tooling/Transformer/SourceCode.cpp:169
+ if (contains(Terminators, Tok))
+ Terminated = true;
+ End = Tok.getEndLoc();
----------------
gribozavr2 wrote:
> What if the token is not a terminator? It seems like the loop will keep
> looking for a terminator, but I don't understand why -- based on the doc
> comment, it seems like the intent is that this function will only extend the
> range with extra terminators.
The intent is also to extend with comments and whitespace. Since the lexer is
keeping whitespace (line 116), the whitespace will show up as tokens. I do not
believe there is any documentation on the semantics of this mode, fwiw -- I had
to read the code. Let me know if you think it would help to spell out more
about the operation of the keep-whitespace mode.
================
Comment at: clang/lib/Tooling/Transformer/SourceCode.cpp:196
+ case tok::semi:
+ case tok::comma:
+ if (contains(Terminators, Tok)) {
----------------
gribozavr2 wrote:
> I don't understand this case. We can only reach this loop if we already found
> one terminator. If semicolon is a terminator, fine, we can consume an extra
> one. However, if comma is a terminator, if we enter this "case tok::comma",
> it means we found two commas in a row, which is probably a syntax error.
I've added comments and additional logic (captured by `TerminatedByMacro`) to
explain/refine this case. PTAL.
================
Comment at: clang/lib/Tooling/Transformer/SourceCode.cpp:352
+ // If we didn't see '[[' or '__attribute' it's probably coming from a
+ // macro expansion which is already handled by getExpansionRange(),
+ // below.
----------------
gribozavr2 wrote:
> s/getExpansionRange/makeFileCharRange/? Although I don't think it does that,
> because its comment says "... the function will fail because the range
> overlaps with only a part of the macro".
>
>
`makeFileCharRange` is right, even if it will result in an error. Basically, if
the attribute is the start of the macro expansion, then `makeFileCharRange`
will expand it to an valid range. Otherwise, it will return an invalid range,
so we will as well. I've extended the comments in the header to call out this
possibility of returning an invalid range.
================
Comment at: clang/unittests/Tooling/SourceCodeTest.cpp:150
+ struct VarDeclsVisitor : TestVisitor<VarDeclsVisitor> {
+ llvm::Annotations Code;
+
----------------
gribozavr2 wrote:
> If you feel motivated, it could be beneficial to lift this member into
> TestVisitor itself (and change RunOver to accept llvm::Annotations).
Possibly. For now, I've lifted into a class template shared by most of the new
tests. I think it would be good to followup with a patch that moves it to
TestVisitor but that may take more design work than I want to bundle in this
patch.
================
Comment at: clang/unittests/Tooling/SourceCodeTest.cpp:231
+ // Includes comments even in the presence of trailing whitespace.
+ Visitor.Code = llvm::Annotations("$r[[// Comment.\nint x = 4;]] ");
+ Visitor.runOverWithComments(Visitor.Code.code());
----------------
gribozavr2 wrote:
> But there's no whitespace.
the whitespace trails the *decalaration*.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72153/new/
https://reviews.llvm.org/D72153
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
