sammccall added inline comments.
================ 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. ---------------- 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... ================ Comment at: clang-tools-extra/clangd/SourceCode.h:295 const MacroInfo *Info; + /// Subject to #line directive substitution. E.g. a macro defined in a + /// built-in file might have an identifier location inside the main file. ---------------- It's not obvious to me that this comment describes a special case but the field is valid in general. Maybe ``` /// Location of the identifier that names the macro. /// Unlike Info->Location, this translates preamble-patch locations to main-file locations. ``` ================ Comment at: clang-tools-extra/clangd/SourceCode.h:297 + /// built-in file might have an identifier location inside the main file. + SourceLocation IdentLoc; }; ---------------- nit: call this NameLoc or NameLocation? ident is *slightly* vague Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80198/new/ https://reviews.llvm.org/D80198 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
