kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:369
+ Inputs.Index && !BuildDir.getError()) {
auto Style = getFormatStyleForFile(Filename, Inputs.Contents, VFS.get());
auto Inserter = std::make_shared<IncludeInserter>(
----------------
while here, maybe introduce another span for include fixer initialization ?
as `getFormatStyle` can do IO
================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:374
if (Preamble) {
for (const auto &Inc : Preamble->Includes.MainFileIncludes)
Inserter->addExisting(Inc);
----------------
umm, actually this is wrong in presence of stale preambles ... we should
perform patching before initializing include fixer and make use of patched
includes rather than stale ones.
no action needing, just taking a note for myself.
================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:434
Clang->getASTContext().setTraversalScope(ParsedDecls);
- {
+ if (CTFinder.hasValue()) {
// Run the AST-dependent part of the clang-tidy checks.
----------------
maybe
```
if (ProduceDiagnostics) {
assert(CTFinder.hasValue());
...
}
```
to keep it similar to other usages.
================
Comment at: clang-tools-extra/clangd/TUScheduler.h:113
/// Indicates whether we reused the prebuilt AST.
+ /// FIXME: always false, diagnostics are always built from fresh ASTs.
bool ReuseAST = false;
----------------
why don't we just drop this ? I believe this patch is doing 2 things:
- Don't build diagnostics for read operations
- Don't reuse cached asts for diagnostics
and I think getting rid of `ReuseAST` bit would belong to second one. (unless
we decide to emit status while building asts for read operations)
================
Comment at: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp:470
+TEST(ParsedASTTest, NoDiagnostics) {
+ static bool TidyCheckIsError;
+ // A simple clang-tidy check, which we can verify:
----------------
nit: initialize to false
================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:540
+ // Build one file in advance.
+ S.update(Foo, getInputs(Foo, SourceContents.str()), WantDiagnostics::Yes);
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
----------------
WantDiags::Auto should be enough here, as we are blocking afterwards. I've
heard someone is tryin to get rid of those :P
================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:546
+ // Build two more files.
+ S.update(Bar, getInputs(Foo, SourceContents.str()), WantDiagnostics::Yes);
+ S.update(Baz, getInputs(Foo, SourceContents.str()), WantDiagnostics::Yes);
----------------
again WantDiags::Auto should do for both
================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:796
[](std::vector<Diag>) { ADD_FAILURE() << "Should not be called."; });
S.runWithAST("touchAST", FooCpp, [](Expected<InputsAndAST> IA) {
// Make sure the AST was actually built.
----------------
is it worth asserting here (or in a new test) that AST.diags() is empty,
whereas it is non-empty below ? (ofc, after introducing some diags to the code)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81069/new/
https://reviews.llvm.org/D81069
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
