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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits