kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/Preamble.cpp:148 bool operator==(const TextualPPDirective &RHS) const { - return std::tie(DirectiveLine, Text) == - std::tie(RHS.DirectiveLine, RHS.Text); + return std::tie(DirectiveLine, Text, Offset) == + std::tie(RHS.DirectiveLine, RHS.Text, RHS.Offset); ---------------- nit: i'd put offset before text, since it's faster to compare. ================ Comment at: clang-tools-extra/clangd/Preamble.cpp:533 +PreamblePatch PreamblePatch::createMacroPatch(llvm::StringRef FileName, + const ParseInputs &Modified, ---------------- sorry for not being explicit about this on the first round but rather than duplicating the logic, can we have a shared private factory function that takes an extra parameter to control whether includes/macros should be included in the patch? It can probably look something like this: ``` class PreamblePatch { public: enum class PatchType { MacroDirectives, All, }; // these two will just call the create with relevant patchtype static PreamblePatch createFullPatch(FileName, Modified, Baseline); static PreamblePatch createMacroPatch(FileName, Modified, Baseline); private: static PreamblePatch create(filename, Modified, Baseline, PatchType); } ``` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108045/new/ https://reviews.llvm.org/D108045 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits