kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land.
thanks for bearing with me, let's ship it! ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:456 + tooling::IncludeStyle IncludeStyle; + auto DefaultStyle = format::getStyle( + format::DefaultFormatStyle, AST.tuPath(), format::DefaultFallbackStyle, ---------------- nit: this could be shorter with ``` auto FileStyle = format::getStyle(..); if (!FileStyle) { elog("..."); FileStyle = format::getLLVMStyle(); } tooling::HeaderIncludes HeaderIncludes(AST.tuPath(), Code, FileStyle->IncludeStyle); ``` ================ Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:448 + auto Range = MainFile.range("b"); + auto Start = positionToOffset(MainFile.code(), Range.start); + EXPECT_FALSE(Start.takeError()); ---------------- 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_cast`s are also redundant ================ Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:458 + EXPECT_THAT(Findings.MissingIncludes, ElementsAre(BInfo)); + EXPECT_TRUE(BDecl); +} ---------------- 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). 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