gribozavr2 added inline comments.
================
Comment at: clang/lib/Tooling/Transformer/SourceCode.cpp:128
+ // First, lex to the current token (which is the last token of the range that
+ // we know to be deleted. Then, we process the first token separately from
the
+ // rest based on conditions that hold specifically for that first token.
----------------
Parenthetical is not closed.
================
Comment at: clang/lib/Tooling/Transformer/SourceCode.cpp:169
+ if (contains(Terminators, Tok))
+ Terminated = true;
+ End = Tok.getEndLoc();
----------------
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.
================
Comment at: clang/lib/Tooling/Transformer/SourceCode.cpp:196
+ case tok::semi:
+ case tok::comma:
+ if (contains(Terminators, Tok)) {
----------------
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.
================
Comment at: clang/lib/Tooling/Transformer/SourceCode.cpp:294
+ Range.setBegin(T->getBeginLoc());
+ }
+
----------------
Please add tests for templates. (In particular, out-of-line definitions of
member templates, which have two template argument lists.)
================
Comment at: clang/lib/Tooling/Transformer/SourceCode.cpp:312
+ // other entities the comment could refer to), and
+ // * it is not a IfThisThenThat lint check.
+ if (SM.isBeforeInTranslationUnit(Comment->getBeginLoc(),
----------------
Need tests for cases described by this comment.
================
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.
----------------
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".
================
Comment at: clang/unittests/Tooling/SourceCodeTest.cpp:150
+ struct VarDeclsVisitor : TestVisitor<VarDeclsVisitor> {
+ llvm::Annotations Code;
+
----------------
If you feel motivated, it could be beneficial to lift this member into
TestVisitor itself (and change RunOver to accept llvm::Annotations).
================
Comment at: clang/unittests/Tooling/SourceCodeTest.cpp:164
+
+ // Includes newline.
+ Visitor.Code = llvm::Annotations("$r[[int x = 4;]]");
----------------
s/newline/semicolon/
================
Comment at: clang/unittests/Tooling/SourceCodeTest.cpp:211
+ }
+ bool runOverWithComments(StringRef Code) {
+ std::vector<std::string> Args = {"-std=c++11", "-fparse-all-comments"};
----------------
It assumes that it is invoked with the code that corresponds to this->Code.
Therefore the argument is redundant, I think.
================
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());
----------------
But there's no whitespace.
================
Comment at: clang/unittests/Tooling/SourceCodeTest.cpp:258
+ // Does not include comments when either decl or comment are inside macros
+ // (unless both are in the same macro).
+ // FIXME: Change code to allow this.
----------------
"Does not include comments when only the decl or the comment come from a macro."
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
