sammccall added inline comments.
================ Comment at: clang-tools-extra/clangd/TidyProvider.h:25 + /*Filename=*/llvm::StringRef, + /*CWD*/ PathRef) const>; + ---------------- sammccall wrote: > njames93 wrote: > > sammccall wrote: > > > CWD is now effectively unused and can be dropped > > So I tried removing CWD and it leads to assertions failing in the tests(the > > ones with the extension `.test`). > > I do know that the TFS has some issues in those tests anyway: > > ``` > > [build] E[16:04:40.948] VFS: failed to set CWD to /clangd-test: No such > > file or directory.``` > > I could remove the CWD but not have assertions, instead just bail out if we > > encounter those cases, maybe with a log. WDYT? > It's a bit hard to say without more details - what assertions/cases? > > AFAICT: > - Filename now only ever has the value passed from ParsedAST::build(). > - That should always be absolute > - CWD is only used for the filesystem examining Filename and its parents, > and if they're absolute it should be irrelevant > One of these is clearly not true, though :-) (FWIW I patched in the latest revision of this patch, deleted CWD and check-clangd passed without assertion failures) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91029/new/ https://reviews.llvm.org/D91029 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
