kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/Preamble.cpp:625 +// same spelling. +static std::vector<Diag> patchDiags(llvm::ArrayRef<Diag> BaselineDiags, + const ScannedPreamble &BaselineScan, ---------------- sammccall wrote: > I think this function is too long with too many local lambdas, consider > converting it to a class instead (call me old-fashioned!) well it's gonna be an equally long class, but sure :D ================ Comment at: clang-tools-extra/clangd/Preamble.cpp:645 + auto ModifiedStartIt = + ModifiedContentsToLine.find(BaselineScan.Lines[BaselineStart]); + if (ModifiedStartIt == ModifiedContentsToLine.end()) ---------------- sammccall wrote: > we're assuming that the ranges are within the preamble and everyone agrees > about the bounds. If not, BaselineStart may not be a valid index into > BaselineScan.Lines > > This assumption seems broadly reasonable, but is *very much* not locally > enforced. I'd suggest a defensive check or at least an assert. oops, thanks! both here and also the multi-ine check below. ================ Comment at: clang-tools-extra/clangd/Preamble.cpp:648 + return std::nullopt; + int Closest = ModifiedStartIt->second.front(); + for (auto AlternateLine : ModifiedStartIt->second) { ---------------- sammccall wrote: > this doesn't look right: you're first deciding which possible starting point > is closest, and then deciding whether it matches. So a range that matches can > be masked by a range where only the first line matches, if the latter is > closer. i thought because this would be rare it's fine to not do that, but the same applies in the other direction as well :D moved the content based matching inside the loop to consider a line as alternative only after it matches the contents. ================ Comment at: clang-tools-extra/clangd/Preamble.cpp:706 + Diag NewDiag; + // Copy all fields but Notes, Fixes, Name and Tags. + static_cast<DiagBase &>(NewDiag) = static_cast<DiagBase>(D); ---------------- sammccall wrote: > reasoning about the fields you're *not* rewriting feels fragile. (Here and in > TranslateFix). > > Consider copying the whole object and mutating in place (`bool > TranslateDiag(Diag&)` together with `erase_if`) well i am a little worried about copying around notes/fixes just to drop them again (which can have quite some strings). but probably rare enough in practice to not matter. ================ Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:820 + { + // Note is also dropped if diag is gone. + Annotations Code(R"( ---------------- sammccall wrote: > i'm confused about what this comment is getting at - a note without a diag is > not even representable well, it is to make sure we're not promoting an orphaned note to a main diag. updated the comment but happy to drop if you think it's confusing rather than being helpful. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143096/new/ https://reviews.llvm.org/D143096 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits