kadircet added a comment.
thanks! looks amazing, we're missing a little bit of test coverage though
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:280
+bool isFilteredByConfig(const Config &Cfg, llvm::StringRef HeaderSpelling) {
+ // Convert the path to Unix slashes and try to match against the filter.
----------------
s/HeaderSpelling/HeaderPath
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:282
+ // Convert the path to Unix slashes and try to match against the filter.
+ llvm::SmallString<64> Path(HeaderSpelling);
+ llvm::sys::path::native(Path, llvm::sys::path::Style::posix);
----------------
s/Path/NormalizedPath
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:414
+
+std::string findResolvedPath(const include_cleaner::Header &SymProvider) {
+ std::string ResolvedPath;
----------------
what about just `resolvedPath`, if you'd rather keep the verb, i think `get`
makes more sense than `find`. we're not really searching anything.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:418
+ case include_cleaner::Header::Physical:
+ ResolvedPath = SymProvider.physical()->tryGetRealPathName();
+ break;
----------------
nit: you can directly `return SymProvider.physical()->tryGetRealPathName();`
(same for other 2 cases) and have an `llvm_unreachable("Unknown symbol kind");`
after the switch statement.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:421
+ case include_cleaner::Header::Standard:
+ ResolvedPath = SymProvider.standard().name();
+ break;
----------------
in this and the next case we need to trim `<>"`
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:430
+
+std::string findSymbolName(const include_cleaner::Symbol &Sym) {
+ std::string SymbolName;
----------------
same as above, either just `symbolName` or `get`
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:434
+ case include_cleaner::Symbol::Macro:
+ SymbolName = Sym.macro().Name->getName();
+ break;
----------------
again you can just return here and below
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:434
+ case include_cleaner::Symbol::Macro:
+ SymbolName = Sym.macro().Name->getName();
+ break;
----------------
kadircet wrote:
> again you can just return here and below
`getName` is a StringRef, and unfortunately there are some platforms (like
darwin) that don't support implicit conversion from stringrefs to std::string.
so can you call `.str()` explicitly in the end?
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:754
+ IncludeCleanerFindings Findings;
+ if (Cfg.Diagnostics.MissingIncludes != Config::IncludesPolicy::None ||
+ Cfg.Diagnostics.UnusedIncludes != Config::IncludesPolicy::None) {
----------------
i think for now this should be
```
if (Cfg.Diagnostics.MissingIncludes == Config::IncludesPolicy::Strict ||
Cfg.Diagnostics.UnusedIncludes == Config::IncludesPolicy::Experiment) {
```
otherwise we'll run both legacy and new analysis for `UnusedIncludes == Strict`
================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:449
+ include_cleaner::Symbol B{*D};
+ syntax::FileRange BRange{SM, B.declaration().getBeginLoc(), 1};
+ include_cleaner::Header Header{*SM.getFileManager().getFile("b.h")};
----------------
this is pointing at the declaration inside `b.h` not to the reference inside
the main file. are you sure this test passes?
================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:469
+ $d[[d]]();
+ $f[[f]]();
+ })cpp");
----------------
can you also add a reference (and declaration) for std::vector, and have an
IWYU private pragma in one of the headers to test code paths that spell
verbatim and standard headers? also having some diagnostic suppressed via
`IgnoreHeaders` is important to check
================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:481
+ TU.AdditionalFiles["system/e.h"] = guard("#include <f.h>");
+ TU.AdditionalFiles["system/f.h"] = guard("void f();");
+ TU.ExtraArgs.push_back("-isystem" + testPath("system"));
----------------
can you make one of these names qualified? e.g. `namespace ns { struct Bar {
void f(); }; }`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143496/new/
https://reviews.llvm.org/D143496
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
