[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

sammccall added a comment. Oops, I misread and forgot one point. The public interface can't (only) take `optional` because that's not implicitly convertible from a std::string which is what we usually want to pass. Thus the silly overloads, which we were going to fix with a more careful

[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

sammccall added a comment. > And then in class RealFileSystemProvider, you want to override > doGetDefaultFileSystem() while leaving doGetFileSystem untouched. Does that > basically reflect your intent here? Yes, except with fewer confusing names :-) The name used now (there was a rename

[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

Quuxplusone added inline comments. Comment at: clang-tools-extra/clangd/support/FSProvider.h:37 + /// This is an overload instead of an optional to make implicit string -> + /// StringRef conversion possible. + virtual llvm::IntrusiveRefCntPtr Re how to fix

[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

bjope added a comment. I think the warnings can be hidden by something like this (I'm no expert in this area though, but it seems like this technique has been used a number of times before in llvm, so hopefully it applies here as well): diff --git a/clang-tools-extra/clangd/Preamble.cpp

[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

uabelho added a comment. In D81920#2109901 , @kadircet wrote: > Can you check if you see the warning with different versions of gcc? Originally I got them with 7.4.0 and now I tried with 9.3.0 and got them there as well. Repository: rG LLVM Github

[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

kadircet added a comment. thanks for the info @uabelho! this looks like a dormant warning though, as StringRef is not implicitly convertible to NoneType (and vice-versa) hence anyone trying to make use of the hidden overload would get a hard compile error anyways. Moreover this class is mostly

[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

uabelho added inline comments. Comment at: clang-tools-extra/clangd/support/FSProvider.h:39 + virtual llvm::IntrusiveRefCntPtr + getFileSystem(PathRef CWD) const; }; I noticed that gcc (7.4) warns on this line: ```

[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

uabelho added inline comments. Comment at: clang-tools-extra/clangd/Preamble.cpp:242 llvm::IntrusiveRefCntPtr - getFileSystem() const override { + getFileSystem(llvm::NoneType) const override { return VFS; A warning here too ```

[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

This revision was automatically updated to reflect the committed changes. Closed by commit rG2dc2e47e3cb7: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

kadircet updated this revision to Diff 271354. kadircet marked an inline comment as done. kadircet added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81920/new/ https://reviews.llvm.org/D81920 Files:

[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

kadircet marked 9 inline comments as done. kadircet added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:555 // different code layout. if (auto CorrespondingFile = getCorrespondingHeaderOrSource( + std::string(Path),

[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:323 +format::DefaultFallbackStyle, Code, +

[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

kadircet updated this revision to Diff 271328. kadircet added a comment. - Provide two overloads to make implicit string -> StringRef conversion possible. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81920/new/ https://reviews.llvm.org/D81920

[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

kadircet added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:187 +Opts.ClangTidyOpts = +GetClangTidyOptions(*FSProvider.getFileSystem("."), File); Opts.SuggestMissingIncludes = SuggestMissingIncludes; sammccall wrote: >

[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

kadircet updated this revision to Diff 271312. kadircet marked 2 inline comments as done. kadircet added a comment. - Change signature to llvm::Optional to accomodate call sites that don't want to cd Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

sammccall added a comment. Looks like an improvement but I don't like the `(".")` in various places. Maybe make the param optional and don't cd if it's none? I wouldn't give it a default arg though, the idea is to force a choice. Comment at:

[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, ilya-biryukov. Herald added a project: clang. We've faced a couple of problems when the returned FS didn't have the proper working directory. New