sammccall added a comment. In D82729#2121999 <https://reviews.llvm.org/D82729#2121999>, @kadircet wrote:
> Yes it does, but tests that I updated are also adding a "foo.cpp" file, which > tries to include some headers. Those headers can only be retrieved from FS > and not from the dirty buffer (that has been populated via > Server.AddDocument). > Hence even though this change should be invisible for the outcome of the > tests (as symbols will still be populated due to indexing of headers), this > ensures we've restored them to the proper state after > d26721776ff08e0ecdd73b8851c44032d7c0a366 > <https://reviews.llvm.org/rGd26721776ff08e0ecdd73b8851c44032d7c0a366> > I don't think these tests ever wanted to index the headers directly (as main > files), they rather intended the headers to be indexed through `foo.cpp` > including them, but I wasn't sure about that assumption hence didn't make > that change. Ah right, I think that change is safe and a good idea (assuming tests pass). Essentially before the test is doing the wrong thing for setup, changing it to doing the right thing seems better than changing it to do both! >> I think a slightly more principled fix here is that we should just use >> TestTU + clangd::getWorkspaceSymbols directly and stop using ClangdServer. >> We can land this as-is if tests are flaking and need a quick fix and TestTU >> is too invasive, but it seems it shouldn't be complicated. WDYT? > > that option also SG, but I think there's value in having a couple tests that > are invoked through TUScheduler to test the full production behaviour. > So I would at least copy some exemplar ones to ClangdTests. Maybe one... isn't this covered in our lit tests though? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82729/new/ https://reviews.llvm.org/D82729 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
