[PATCH] D72153: [libTooling] Add function to determine associated text of a declaration.

[Dmitri Gribenko via Phabricator via cfe-commits]

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