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
