[PATCH] D91029: [clangd] Implement clang-tidy options from config

Wed Nov 25 09:01:02 PST 2020

njames93 added inline comments.

================
Comment at: clang-tools-extra/clangd/TidyProvider.h:25
+                                                /*Filename=*/llvm::StringRef,
+                                                /*CWD*/ PathRef) const>;
+
----------------
sammccall wrote:
> 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)
Oh I added some assertions to ensure that it was a directory, those are the ones that failed. The ones ensuring its absolute ran without a hitch.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91029/new/

https://reviews.llvm.org/D91029