[PATCH] D80198: [clangd] locateMacroAt handles patched macros

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:123
 
+// Spells directive in \p DirectiveRange while prepending it with \p Prefix.
+// Returned string can be used with a #line directive on line \p DirectiveLine
----------------
> Spells directive in \p DirectiveRange while prepending it with \p Prefix

I wouldn't understand what this meant if I didn't already know :-) Maybe:

```
// Formats a PP directive consisting of Prefix (e.g. "#define ") and Body ("X 10").
// The formatting is copied so that the tokens in Body have PresumedLocs with
// correct columns and lines.
```


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:127
+std::string spellDirective(llvm::StringRef Prefix, SourceRange DirectiveRange,
+                           unsigned &DirectiveLine, const SourceManager &SM) {
+  std::string SpelledDirective;
----------------
nit: move out-param to end?


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:133
+  auto DecompLoc = SM.getDecomposedLoc(DirectiveRange.getBegin());
+  DirectiveLine = SM.getLineNumber(DecompLoc.first, DecompLoc.second);
+  auto SpaceBefore = SM.getColumnNumber(DecompLoc.first, DecompLoc.second) - 1;
----------------
This is only going to give you something useful for file IDs, not macro expansions. Whose responsibility is it to make sure there's no macro IDs here?


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:134
+  DirectiveLine = SM.getLineNumber(DecompLoc.first, DecompLoc.second);
+  auto SpaceBefore = SM.getColumnNumber(DecompLoc.first, DecompLoc.second) - 1;
+
----------------
this name seems a bit confusing to me, since it only covers one case.
TargetColumn?
`if (TargetColumn < Prefix.size())` reads a lot celarer I think.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:136
+
+  // Pad with spaces before DirectiveRange to make sure it will be on right
+  // column when patched.
----------------
again this comment only applyes to one branch


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:139
+  if (SpaceBefore < Prefix.size()) {
+    // Prefix was longer than the space we had. Pad from start of a new line.
+    OS << "\\\n" << std::string(SpaceBefore, ' ');
----------------
Rather than "pad from the start of a new line" it might be clearer to give an example here:
```
... We produce e.g.:
#line N-1
#define \
   X 10
```


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:149
+  }
+  OS << toSourceCode(SM, DirectiveRange);
+  return OS.str();
----------------
this asserts isValidFileRange, so we need this to be true :-)


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:976
+  // #line directives to hint their original location in main file.
+  auto DefFile = SM.getFileID(NameLoc);
+  auto IncludeLoc = SM.getIncludeLoc(DefFile);
----------------
I think this block can now be lifted to a function in SourceCode or Preamble or so?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80198