kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:445
-Fix removeAllUnusedIncludes(llvm::ArrayRef<Diag> UnusedIncludes) {
+Fix removeAllUnusedIncludes(llvm::ArrayRef<Diag> UnusedIncludes,
+ llvm::StringRef Code) {
----------------
what about just taking in `llvm::ArrayRef<const Inclusion *> UnusedIncludes`
and having a `TextEdit dropInclude(const Inclusion&);` that we can use in here
& inside `generateUnusedIncludeDiagnostics`? That way we can just use the
spelling inside the inclusion to generate the `label`, without all the extra
logic.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:485
// newText (#include "my_missing_header.h") -> TextEdit.
- llvm::StringMap<TextEdit> Edits;
+ llvm::StringMap<std::pair<TextEdit, const MissingIncludeDiag*>> Edits;
for (const auto &Diag : MissingIncludeDiags) {
----------------
i think rather than a single diag info, we should actually store a set of them
now, right? as we want to surface not only the "first" use of a header, but
all. (possibly with some trimming).
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:514
+ /*description=*/
+ llvm::formatv("Line {0}: '{1}'", StartLine, Lines[StartLine])}});
}
----------------
i don't think line information is as useful (as there's no structured link to
it). what about something like:
```
label: `Include "foo.h"`
description: `Provides 'X', 'Y', 'Z', ...`
```
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:550
std::optional<Fix> FixAll;
if (RemoveAllUnused && AddAllMissing)
FixAll = fixAll(*RemoveAllUnused, *AddAllMissing);
----------------
sorry for missing it during last review, but this should actually be `if
(!UnusedIncludes.empty() && !MissingIncludeDiags.empty())` as even if we have
only 1 fix for each category, we should still show the fix all. probably it
deserves a fix in a separate patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149437/new/
https://reviews.llvm.org/D149437
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
