[PATCH] D143496: [clangd] Add support for missing includes analysis.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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