[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch
Kadir Cetinkaya via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Feb 17 03:19:38 PST 2023
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
More information about the cfe-commits
mailing list