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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Thanks for the detailed review, especially given the complexity of the code!



================
Comment at: clang/lib/Tooling/Transformer/SourceCode.cpp:152
+    // will not break anything that removing the entity wouldn't have
+    // already broken.
+  bool TerminatedByMacro = false;
----------------
gribozavr2 wrote:
> Indent -2.
I re-read this comment and I don't think it's helpful -- it's a holdover from the usecase where the code is deleting the decl,. instead of its more general use of just identifying the associated text. Moreover, the comment that follows does a full explanation of the role of this variable. So, I've deleted the comment and moved the declaration to follow the comment and lead the block of code that follows, which seems the appropriate location.


================
Comment at: clang/lib/Tooling/Transformer/SourceCode.cpp:156
+  // 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.
----------------
gribozavr2 wrote:
> s/that we know to be deleted/that is being deleted/
Thanks. Here too I rewrote the comment a bit to avoid discussing deletion specifically.


================
Comment at: clang/lib/Tooling/Transformer/SourceCode.cpp:282
+                             const LangOptions &LangOpts) {
+  // If the first character is a newline, we'll check for an empty line as a
+  // separator. However, we can't identify an empty line using tokens, so we
----------------
gribozavr2 wrote:
> I'm not sure what code implements the "if the first character is a newline" logic. The for loop below checks if LocChars starts with horizontal whitespace followed by vertical whitespace.
> 
> Also, I don't see where we find an *empty* line. I understand an empty line as zero or more horizontal whitespace characters between two vertical whitespace characters.
Good catch. The comment hadn't been updated since the first version, while the code has changed. There's an implicit invariant on how it's called that guarantees that if the rest of the line is whitespace and newline then the preceding character was a newline.

However, I think that's far too complicated an invariant to rely on implicitly, so I've updated the code to check it explicitly. Now, it only relies on Loc not being the first of the file (which seems reasonable given that this function is used for exclusive end locations -- meaning, there should always be something that precedes them).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72153/new/

https://reviews.llvm.org/D72153