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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
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;
----------------
sammccall wrote:
> 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?
For `define` directives this is already true as `MacroInfo`'s definition range is a file range(as macro bodies are lexed without macro expansions).

For any other directive with macro expansions, we only care about the expansion locations as we want to spell out the directive as written. So we can just map back to expansion locs here.


================
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.
----------------
sammccall wrote:
> again this comment only applyes to one branch
why ? we pad in both cases, starting from the beginning of the line in first branch and starting after Prefix in the second.


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:975
+
+  // Macro definitions could be injected through preamble patch. These contain
+  // line directives to hint their original location in main file.
----------------
sammccall wrote:
> OK, this is fairly horrible...
> 
> I want to say the preamble patch location translation should be a separate function rather than coupled to macro-specific stuff.
> (Then we don't even need the extra struct member, but we can still keep it to "remind" people the translation is needed, if we like).
> 
> But of course we didn't preserve the formatting in the preamble patch, so `getPresumedLoc()` doesn't give us the right location, it just gives us something on the right line that we then need to re-parse...
> 
> This really isn't looking like a great tradeoff to me anymore (and I know I suggested it).
> How much work do you think it is (compared to say, the logic here plus doing it in ~2 more places) to give the preamblepatch the invariant that the PresumedLoc has a usable col as well.
> The original implementation did padding with spaces, but I guess we could as well have the preamble patching stuff splice the actual source code...
as discussed offline, changed patching logic to put directive in correct column in addition to line. Now we can just make use of presumedloc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80198