sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:678
+ if (Preamble) {
+ auto PDiags = Patch->patchedDiags();
+ Diags->insert(Diags->end(), PDiags.begin(), PDiags.end());
----------------
llvm::append_range(Diags, Patch->patchedDiags()) ?
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:373
SP.Includes = std::move(Includes.MainFileIncludes);
+ for (auto Pos = Contents.find('\n'); Pos != Contents.npos;
+ Contents = Contents.substr(Pos + 1), Pos = Contents.find('\n')) {
----------------
llvm::append_range(SP.Lines, llvm::split(Contents, "\n")) ?
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:639
+// Checks if all pointers in \p D are for the same line of the main file.
+static bool diagReferencesMultipleLines(const Diag &D) {
----------------
nit: comment sense is the opposite of the function name
maybe say why this is an important property, or give the function a
higher-level name?
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:640
+// Checks if all pointers in \p D are for the same line of the main file.
+static bool diagReferencesMultipleLines(const Diag &D) {
+ int Line = D.Range.start.line;
----------------
this function + translateDiag look like a validate/parse pair that need to be
kept in sync.
they could be a single `translateDiag()` function that could fail, instead.
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:645
+ bool NotePointsToOutside = llvm::any_of(D.Notes, [&](const Note &N) {
+ return N.File == D.File &&
+ (N.Range.start.line != Line || N.Range.end.line != Line);
----------------
this check looks dubious to me:
- `File` is a human-readable string with ill-defined format, comparing them
for equality isn't safe, you want InsideMainFile instead
- if the note points to another file, then it's not that translation is
impossible, but rather no translation is needed
- if the note points to the same file, then why can't we apply the same
translation?
I think this should rather be something like:
```
bool translateDiag(DiagBase& D, bool IsMainDiag) {
if (IsMainDiag) {
for (const auto& N : cast<Diag>(D).Notes) {
if (!translateDiag(N, /*IsMainDiag=*/false))
return false;
}
}
if (D.InsideMainFile) {
// attempt patching range of D itself
}
}
```
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:680
+static llvm::DenseMap<int, llvm::SmallVector<const Diag *>>
+mapDiagsToLines(llvm::ArrayRef<Diag> Diags) {
+ llvm::DenseMap<int, llvm::SmallVector<const Diag *>> LineToDiags;
----------------
nit: name seems backwards (but as noted below I think this function can go away
entirely)
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:705
+// same spelling.
+static std::vector<Diag> patchDiags(llvm::ArrayRef<Diag> BaselineDiags,
+ const ScannedPreamble &BaselineScan,
----------------
If I'm understanding the algorithm right:
- we make a map of line content => original line number
- we iterate through each line of the modified preamble, and:
- find the corresponding original line
- look for diagnostics to translate
Why not loop over diagnostics instead of lines in the modified preamble? There
should be a lot fewer, and you avoid the lines-to-diags map.
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:707
+ const ScannedPreamble &BaselineScan,
+ const ScannedPreamble &ModifiedScan) {
+ std::vector<Diag> PatchedDiags;
----------------
large preamble with no diagnostics seems pretty plausible, maybe bail out early
to avoid building the content map?
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:795
// Baseline for exclusion.
- llvm::DenseMap<std::pair<tok::PPKeywordKind, llvm::StringRef>,
- const Inclusion *>
- ExistingIncludes;
- for (const auto &Inc : Baseline.Includes.MainFileIncludes)
- ExistingIncludes[{Inc.Directive, Inc.Written}] = &Inc;
- // There might be includes coming from disabled regions, record these for
- // exclusion too. note that we don't have resolved paths for those.
- for (const auto &Inc : BaselineScan->Includes)
- ExistingIncludes.try_emplace({Inc.Directive, Inc.Written});
+ auto ExistingIncludes =
+ getExistingIncludes(*BaselineScan, Baseline.Includes.MainFileIncludes);
----------------
I think this change and the IncludeKey class can be reverted now?
================
Comment at: clang-tools-extra/clangd/Preamble.h:161
+ /// Returns diag locations for Modified contents, only contains diags
attached
+ /// to an #include or #define directive.
----------------
the stuff about `#include`/`#define` doesn't seem to correspond to anything in
the code (obsolete?)
and below
================
Comment at: clang-tools-extra/clangd/Preamble.h:175
std::string PatchFileName;
- /// Includes that are present in both \p Baseline and \p Modified. Used for
- /// patching includes of baseline preamble.
+ // Includes that are present in both \p Baseline and \p Modified. Used for
+ // patching includes of baseline preamble.
----------------
if you want to undoxygen this comment, also remove `\p`?
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
