Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang-tools-extra/clangd/IncludeCleaner.cpp | ||
---|---|---|
751 | Ok should be done now. | |
766 | Thanks. This might very well be the case, but this comment also seems to suggest some premature optimization (in a way). It totally makes sense to re-use what's re-usable, but this sort of refactoring really only makes sense once we have a clear use case (and get there :) | |
clang-tools-extra/clangd/IncludeCleaner.h | ||
40 | Ok, agreed. Storing symbols now. Having only unused include analysis on is a convincing use case. |
thanks! looks amazing, we're missing a little bit of test coverage though
clang-tools-extra/clangd/IncludeCleaner.cpp | ||
---|---|---|
284 | s/HeaderSpelling/HeaderPath | |
286 | s/Path/NormalizedPath | |
418 | 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. | |
422 | 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. | |
425 | in this and the next case we need to trim <>" | |
434 | same as above, either just symbolName or get | |
438 | again you can just return here and below | |
438 | 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? | |
760 | 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 | |
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp | ||
450 | this is pointing at the declaration inside b.h not to the reference inside the main file. are you sure this test passes? | |
470 | 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 | |
482 | can you make one of these names qualified? e.g. namespace ns { struct Bar { void f(); }; } |
Thank you for all the thoughtful comments!
clang-tools-extra/clangd/IncludeCleaner.cpp | ||
---|---|---|
418 | Ok, let's call it get. I do prefer verbs for methods, that's correct. | |
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp | ||
450 | Yes, all the tests pass. | |
470 | Thank you for the great tips on improving test coverage! In fact, I had to also introduce support for private pragmas, as they were not taken care of. Hopefully, the solution will make sense to you. |
thanks, looks great!
clang-tools-extra/clangd/IncludeCleaner.cpp | ||
---|---|---|
453 | we should respect the style configurations (sorry for missing this in previous iterations). you can get the relevant style with: clang::format::getStyle, which has an IncludeStyle. in case the getStyle fails, we should fallback to clang::format::getLLVMStyle as we do in other places. you can get at the relevant VFS instance through sourcemanager. | |
731 | you can directly use !Pragmas->isPrivate(Inc->Resolved) here, instead of getpublic | |
731 | this check seems to be new. what's the reason for rejecting private providers? I can see that we might want to be conservative by not inserting private providers, but treating symbols as unsatisfied when a private provider is already included doesn't feel right. e.g. the code being analyzed might be allowed to depend on this private header, because it's also part of the library, or it's the public header that's exposing this private header. in such a scenario we shouldn't try to insert the public header again. is there a more concrete issue this code is trying to address? | |
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp | ||
445 | nit: braces | |
450 | this is passing because bool BDeclFound; is uninitialized above, if you set it to bool BDeclFound = false; you should see the test fail. there's no declaration for b inside the main file, it's declared in b.h and referenced inside the main file. you still need to search for the decl (without the constraint of being written in main file), use it to build an include_cleaner::Symbol, and use a clangd::Annotation range for the range of the reference. it might be easer to write this as: const NamedDecl* B = nullptr; for (...) { ... B = D; } ASSERT_TRUE(B); // build expected diagnostic info based on B and check that it's equal to what we've produced | |
458 | i think the example for std::vector is solid, and IWYU pragma private needs a little adjustment. | |
471 | we should include private.h through some indirection (not public.h) to check IWYU pragma private spellings are respected. | |
477 | name this range as bar instead of d? | |
481 | could you add a comment here saying this shouldn't be diagnosed? |
Thanks for the comments!
clang-tools-extra/clangd/IncludeCleaner.cpp | ||
---|---|---|
731 | Ok makes sense. No, I guess I was just confused, because I understood that you wanted a test that includes "private.h" with a diagnostic generated saying that "public.h" should be included instead, so I assumed that was expected behaviour. But that's not what you meant, so I misunderstood. | |
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp | ||
450 | Didn't know there was a difference between uninitialized and false.. Thanks for the idea with ASSERT_TRUE(Decl). Please check out the new version. |
clang-tools-extra/clangd/IncludeCleaner.cpp | ||
---|---|---|
456 | creating a copy of LLVM style unnecessarily all the time is not really great, can you move this into the failure case instead? also you can drop the clang:: here and elsewhere, as this code is already part of clang:: namespace. | |
457 | as mentioned above we also need to make sure we're passing the relevant VFS instance inside the source manager, rather than using the real file system (as some clients rely on the VFS). | |
458 | s/MainFile->getName()/AST.tuPath()/ to be consistent with other places. | |
460 | can you also elog this error? as it should be rare and when this goes wrong, having this mentioned in the logs are really useful for debugging (since the failure is actually outside of clangd, it usually means a malformed config file somewhere) | |
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp | ||
428 | nit: instead of using a point, can you use a range here instead (i.e. [[b]])? afterwards you can have a FileRange pointing at both offsets, rather than relying on the length of the identifier. | |
448 | rest of the code here doesn't really belong to the for loop, can you take them out? |
thanks for bearing with me, let's ship it!
clang-tools-extra/clangd/IncludeCleaner.cpp | ||
---|---|---|
456 | nit: this could be shorter with auto FileStyle = format::getStyle(..); if (!FileStyle) { elog("..."); FileStyle = format::getLLVMStyle(); } tooling::HeaderIncludes HeaderIncludes(AST.tuPath(), Code, FileStyle->IncludeStyle); | |
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp | ||
450 | nit: size_t Start = llvm::cantFail(positionToOffset(MainFile.code(), Range.start)); size_t End = llvm::cantFail(positionToOffset(MainFile.code(), Range.end)); no need for EXPECT_FALSE(..takeError())s as llvm::cantFail will fail (no pun intended :P), static_casts are also redundant | |
460 | it'd be better to ASSERT_TRUE(BDecl); right after the for loop, as rest of the code will crash (and even trigger undefined behavior because we're dereferencing nullptr in failure case). difference between ASSERT_X and EXPECT_X macros are, the former will stop execution of the particular test (hence we'll never trigger a nullptr deref with ASSERT_TRUE), whereas the latter just prints the failure, but doesn't abort the execution of test (hence helps print multiple failures at once, when they're non fatal). |
This change broke regression, none-one read: "This revision was landed with ongoing or failed builds."
./ClangdTests.exe/IncludeCleaner/GenerateMissingHeaderDiags tests fails on windows
This breaks tests on windows: http://45.33.8.238/win/75486/step_9.txt
Please take a look and revert for now if it takes a while to fix.
Oh, that was reported a while ago already. Reverted in 2eb5ac99a76dbbf8ac68c538211fabeaa5ac0bfd for now.
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp | ||
---|---|---|
464 | Looks like this filter doesn't work on windows (the / vs \ path separator might be the root cause here), I think a fix can be
|