[clangd] Rename FS.view(None) to FS.viewWithDefaultCWD()

[• Quuxplusone]

Summary

Fixes an instance of -Woverloaded-virtual on GCC.
Clarifies the purpose of this particular function.
Frees up a register that was being used for this pointless parameter of type llvm::NoneType.

Also eliminate virtual from view(PathRef) because it is not intended to be overridden.

(This is how I propose to address the underlying issue that led to D82617, instead of D82617. Of course the final call is Kadir's-or-Sam's-or-ultimately-certainly-not-mine.)

[kadircet]

Thanks, renaming was also another option we had in mind, see https://reviews.llvm.org/D81920#2109901 and possibly the following comments. I thought it was discussed in the disable-the-warning thread, but to elaborate a little more:

Naming is hard in general, this case is no different.

The two overloads in the base class literally do the same thing, one of them doesn't change the CWD. Hence the Default doesn't reflect what it returns, it's likely to be in an arbitrary state. There are some parts of the code that always make use of absolute paths, and it is meaningless for them to have sense of "CWD". Since we can't always create a view with some sane CWD, we needed such an option.

It was originally meant to be a single function with signature view(llvm::Optional<llvm::StringRef>) but this result in wrapping the likely std::string parameter in an explicit llvm::StringRef constructor on almost all callsites, as an optional<stringref> can't be constructed implicitly. Hence we rather chose to split the parameter type into two overloads.

So IMHO, having two functions with different names doesn't reflect the intent as clearly as code's current state (e.g. via overloads). This is definitely subjective though and depends on the taste of people that's reading/maintaining the code in question.

As for dropping the virtual in the latter function, it was done to support a pure virtual implementation of a ThreadsafeFS whom stored the CWD internally, not that we need it today or it is certain that we'll need one someday. So it can be changed when the need arises.

[sammccall]

Yeah, IMO we spent a bunch of time on these names, I still like them, and I don't find the arguments since then convincing. New info is of course the warning, and the lack of consensus to change it at clang level.

As for this proposal specifically

• viewWithDefaultCWD suggests to me the default has some semantics which don't exist, if using this API "shape" I'd substitute Arbitrary here
• I think the argument for changing the *public* API is particularly weak (or missing?). It's more important than the private API, and no change is required to silence the warning. A new name is inevitably a mouthful, and it ties our hands for adding the Optional overload.
• I could certainly live with private: virtual T viewImpl() = 0. Sounds like some will find that nicer, it's not very intrusive, and we don't need to maintain a flag-divergence from clang

[virtual]... not that we need it today or it is certain that we'll need one someday

We have an out-of-tree implementation that should be using this, though may not have switched yet. But this also certainly will never really matter, so let's drop virtual for simplicity.

[• Quuxplusone]

In D82736#2119262, @sammccall wrote:

viewWithDefaultCWD suggests to me the default has some semantics which don't exist, if using this API "shape" I'd substitute Arbitrary here

I'm naturally 100% fine with that. I can continue updating this patch if you like; otherwise my default will be to abandon this patch on the assumption that anything I can do, you can do better. :)

---

• I think the argument for changing the *public* API is particularly weak (or missing?)...

and Kadir wrote:

The two overloads in the base class literally do the same thing ... It was originally meant to be a single function with signature view(llvm::Optional<llvm::StringRef>) but this result in wrapping the likely std::string parameter in an explicit llvm::StringRef constructor on almost all callsites, as an optional<stringref> can't be constructed implicitly. Hence we rather chose to split the parameter type into two overloads.

Well, see, I would call that "the two overloads do completely different things," right? One changes the CWD and the other doesn't. So they don't do "the same thing" and therefore shouldn't be part of the same overload set. Overload sets are specifically used in statically polymorphic code, where you might have a piece of generic code, like a template, that always wants to do "the same thing" but on different argument types. That's never the case here. Also, splitting a single function view(Optional<PathRef>) into a pair of overloads view(PathRef) and view(NoneType) does not work the way you seem to be expecting it to. If you actually had an Optional<PathRef> p, then calling view(p) simply wouldn't compile. Optionals don't "automagically visit" like that (and llvm::Optional's monadic map method doesn't work quite like that either).
The one thing your overload set would theoretically let you do is, with a std::variant,

std::variant<PathRef, NoneType> v;
auto FSP = std::visit([](auto x){ return TFS.view(x); }, v);  // call either view(PathRef) or view(NoneType) depending on the variant's current runtime state

But the current code doesn't ever do anything like that. (Nor should it. It would be confusing.) Since there is no physical reason (e.g. visitor pattern) for the two functions to have the same name, and there is no logical reason (e.g. "doing-the-same-thing-ness") for them to have the same name, therefore in my book they should have different names.

• I could certainly live with private: virtual T viewImpl() = 0. Sounds like some will find that nicer, it's not very intrusive, and we don't need to maintain a flag-divergence from clang

👍 IIUC, you're proposing to keep the overload set, but make the whole overload set public-and-nonvirtual, and have view(NoneType) dispatch to viewImpl(). I still think the overload set is misguided for the reasons given earlier in this reply, but I agree that a public nonvirtual interface backed by a single virtual implementation method would be better stylistically than what's there now, and it would also make GCC happy.

[sammccall]

In D82736#2120089, @Quuxplusone wrote:

In D82736#2119262, @sammccall wrote:

viewWithDefaultCWD suggests to me the default has some semantics which don't exist, if using this API "shape" I'd substitute Arbitrary here

I'm naturally 100% fine with that. I can continue updating this patch if you like; otherwise my default will be to abandon this patch on the assumption that anything I can do, you can do better. :)D82793

Thanks! I put together D82793. Feel free to ignore the below as my "for the record" nitpicking,..

• I think the argument for changing the *public* API is particularly weak (or missing?)...

Well, see, I would call that "the two overloads do completely different things," right? One changes the CWD and the other doesn't.

Not really, that's an implementation detail! The contract is request vs no request of working directory.
With both overloads virtual (current state), the most reasonable implementation for a truly virtual FS is probably for one to return new FSImpl(Path) and the other to call new FSImpl("/"). But both the strategy and the directory chosen are impl-dependent.

Also, splitting a single function view(Optional<PathRef>) into a pair of overloads view(PathRef) and view(NoneType) does not work the way you seem to be expecting it to.

It's a bit rude to assume that if you can't think of a reason, it must be everyone else that's ignorant. To recap what's been previously stated:

• an overload set is required to allow passing both string and NoneType, because string doesn't implicitly convert to Optional<StringRef>.
• large overload sets are prone to ambiguity problems
• we have out-of-tree callers that use yet-different string types
• the change that introduced this interface was primarily a renaming patch that touched many files (and so each change is trivial, but work to revert)
• making the overload set support actual optionals touches few files, but may introduce subtle compile problems and need to be reverted
• therefore we deferred the Optional<PathRef> overload until the dust settled from that patch (it hasn't) and possibly until it's actually needed.

I'm sure there are different ways to do this, but I can assure you that both author and reviewer understand how function calls work!

[• Quuxplusone]

Superseded by D82793. :)

clang-tools-extra/clangd/ClangdServer.cpp

 
 void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
                                llvm::StringRef Version,
                                WantDiagnostics WantDiags, bool ForceRebuild) {
   ParseOptions Opts;
   Opts.ClangTidyOpts = tidy::ClangTidyOptions::getDefaults();
   // FIXME: call tidy options builder on the worker thread, it can do IO.
   if (GetClangTidyOptions)
     Opts.ClangTidyOpts =
-        GetClangTidyOptions(*TFS.view(/*CWD=*/llvm::None), File);
+        GetClangTidyOptions(*TFS.viewWithDefaultCWD(), File);
   Opts.SuggestMissingIncludes = SuggestMissingIncludes;
 
   // Compile command is set asynchronously during update, as it can be slow.
   ParseInputs Inputs;
   Inputs.TFS = &TFS;
   Inputs.Contents = std::string(Contents);
   Inputs.Version = Version.str();
   Inputs.ForceRebuild = ForceRebuild;

 llvm::Expected<std::vector<TextEdit>>
 ClangdServer::formatOnType(llvm::StringRef Code, PathRef File, Position Pos,
                            StringRef TriggerText) {
   llvm::Expected<size_t> CursorPos = positionToOffset(Code, Pos);
   if (!CursorPos)
     return CursorPos.takeError();
   auto Style = format::getStyle(format::DefaultFormatStyle, File,
                                 format::DefaultFallbackStyle, Code,
-                                TFS.view(/*CWD=*/llvm::None).get());
+                                TFS.viewWithDefaultCWD().get());
   if (!Style)
     return Style.takeError();
 
   std::vector<TextEdit> Result;
   for (const tooling::Replacement &R :
        formatIncremental(Code, *CursorPos, TriggerText, *Style))
     Result.push_back(replacementToEdit(Code, R));
   return Result;
void ClangdServer::switchSourceHeader(
     PathRef Path, Callback<llvm::Optional<clangd::Path>> CB) {
   // We want to return the result as fast as possible, strategy is:
   //  1) use the file-only heuristic, it requires some IO but it is much
   //     faster than building AST, but it only works when .h/.cc files are in
   //     the same directory.
   //  2) if 1) fails, we use the AST&Index approach, it is slower but supports
   //     different code layout.
   if (auto CorrespondingFile = getCorrespondingHeaderOrSource(
-          std::string(Path), TFS.view(llvm::None)))
+          std::string(Path), TFS.viewWithDefaultCWD()))
     return CB(std::move(CorrespondingFile));
   auto Action = [Path = Path.str(), CB = std::move(CB),
                  this](llvm::Expected<InputsAndAST> InpAST) mutable {
     if (!InpAST)
       return CB(InpAST.takeError());
     CB(getCorrespondingHeaderOrSource(Path, InpAST->AST, Index));
   };
   WorkScheduler.runWithAST("SwitchHeaderSource", Path, std::move(Action));

clang-tools-extra/clangd/Preamble.cpp

 /// running preprocessor over \p Contents. Returned includes do not contain
 /// resolved paths. \p Cmd is used to build the compiler invocation, which might
 /// stat/read files.
 llvm::Expected<ScannedPreamble>
 scanPreamble(llvm::StringRef Contents, const tooling::CompileCommand &Cmd) {
   class EmptyFS : public ThreadsafeFS {
   public:
     llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
-    view(llvm::NoneType) const override {
+    viewWithDefaultCWD() const override {
       return new llvm::vfs::InMemoryFileSystem;
     }
   };
   EmptyFS FS;
   // Build and run Preprocessor over the preamble.
   ParseInputs PI;
   PI.Contents = Contents.str();
   PI.TFS = &FS;
scanPreamble(llvm::StringRef Contents, const tooling::CompileCommand &Cmd) {
   auto Bounds =
       ComputePreambleBounds(*CI->getLangOpts(), ContentsBuffer.get(), 0);
   auto PreambleContents =
       llvm::MemoryBuffer::getMemBufferCopy(Contents.substr(0, Bounds.Size));
   auto Clang = prepareCompilerInstance(
       std::move(CI), nullptr, std::move(PreambleContents),
       // Provide an empty FS to prevent preprocessor from performing IO. This
       // also implies missing resolved paths for includes.
-      FS.view(llvm::None), IgnoreDiags);
+      FS.viewWithDefaultCWD(), IgnoreDiags);
   if (Clang->getFrontendOpts().Inputs.empty())
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "compiler instance had no inputs");
   // We are only interested in main file includes.
   Clang->getPreprocessorOpts().SingleFileParseMode = true;
   PreprocessOnlyAction Action;
   if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]))
     return llvm::createStringError(llvm::inconvertibleErrorCode(),

clang-tools-extra/clangd/SourceCode.cpp

llvm::Optional<FileDigest> digestFile(const SourceManager &SM, FileID FID) {
   return digest(Content);
 }
 
 format::FormatStyle getFormatStyleForFile(llvm::StringRef File,
                                           llvm::StringRef Content,
                                           const ThreadsafeFS &TFS) {
   auto Style = format::getStyle(format::DefaultFormatStyle, File,
                                 format::DefaultFallbackStyle, Content,
-                                TFS.view(/*CWD=*/llvm::None).get());
+                                TFS.viewWithDefaultCWD().get());
   if (!Style) {
     log("getStyle() failed for file {0}: {1}. Fallback is LLVM style.", File,
         Style.takeError());
     return format::getLLVMStyle();
   }
   return *Style;
 }
 

clang-tools-extra/clangd/index/Background.cpp

for (auto &LS : Result) {
 
       IndexedSymbols.update(LS.AbsolutePath, std::move(SS), std::move(RS),
                             std::move(RelS), LS.CountReferences);
     }
   }
   Rebuilder.loadedShard(LoadedShards);
   Rebuilder.doneLoading();
 
-  auto FS = TFS.view(/*CWD=*/llvm::None);
+  auto FS = TFS.viewWithDefaultCWD();
   llvm::DenseSet<PathRef> TUsToIndex;
   // We'll accept data from stale shards, but ensure the files get reindexed
   // soon.
   for (auto &LS : Result) {
     if (!shardIsStale(LS, FS.get()))
       continue;
     PathRef TUForFile = LS.DependentTU;
     assert(!TUForFile.empty() && "File without a TU!");

clang-tools-extra/clangd/support/ThreadsafeFS.h

 /// different contexts, so either ThreadsafeFS::view or the returned FS may
 /// contain this logic.
 class ThreadsafeFS {
 public:
   virtual ~ThreadsafeFS() = default;
 
   /// Obtain a vfs::FileSystem with an arbitrary initial working directory.
   virtual llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
-  view(llvm::NoneType CWD) const = 0;
+  viewWithDefaultCWD() const = 0;
 
   /// Obtain a vfs::FileSystem with a specified working directory.
   /// If the working directory can't be set (e.g. doesn't exist), logs and
   /// returns the FS anyway.
-  virtual llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
+  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
   view(PathRef CWD) const;
 };
 
 class RealThreadsafeFS : public ThreadsafeFS {
 public:
   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
-      view(llvm::NoneType) const override;
+      viewWithDefaultCWD() const override;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/support/ThreadsafeFS.cpp

private:
   private:
     std::unique_ptr<File> Wrapped;
   };
 };
 } // namespace
 
 llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
 ThreadsafeFS::view(PathRef CWD) const {
-  auto FS = view(llvm::None);
+  auto FS = viewWithDefaultCWD();
   if (auto EC = FS->setCurrentWorkingDirectory(CWD))
     elog("VFS: failed to set CWD to {0}: {1}", CWD, EC.message());
   return FS;
 }
 
 llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
-RealThreadsafeFS::view(llvm::NoneType) const {
+RealThreadsafeFS::viewWithDefaultCWD() const {
   // Avoid using memory-mapped files.
   // FIXME: Try to use a similar approach in Sema instead of relying on
   //        propagation of the 'isVolatile' flag through all layers.
   return new VolatileFileSystem(
       llvm::vfs::createPhysicalFileSystem().release());
 }
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/tool/ClangdMain.cpp

if (EnableClangTidy) {
     auto EmptyDefaults = tidy::ClangTidyOptions::getDefaults();
     EmptyDefaults.Checks.reset(); // So we can tell if checks were ever set.
     tidy::ClangTidyOptions OverrideClangTidyOptions;
     if (!ClangTidyChecks.empty())
       OverrideClangTidyOptions.Checks = ClangTidyChecks;
     ClangTidyOptProvider = std::make_unique<tidy::FileOptionsProvider>(
         tidy::ClangTidyGlobalOptions(),
         /* Default */ EmptyDefaults,
-        /* Override */ OverrideClangTidyOptions, TFS.view(/*CWD=*/llvm::None));
+        /* Override */ OverrideClangTidyOptions, TFS.viewWithDefaultCWD());
     Opts.GetClangTidyOptions = [&](llvm::vfs::FileSystem &,
                                    llvm::StringRef File) {
       // This function must be thread-safe and tidy option providers are not.
       tidy::ClangTidyOptions Opts;
       {
         std::lock_guard<std::mutex> Lock(ClangTidyOptMu);
         // FIXME: use the FS provided to the function.
         Opts = ClangTidyOptProvider->getOptions(File);

clang-tools-extra/clangd/unittests/ClangdTests.cpp

)cpp";
   EXPECT_EQ(DumpParse1, DumpParse2);
   EXPECT_NE(DumpParse1, DumpParseDifferent);
 }
 
 TEST_F(ClangdVFSTest, PropagatesContexts) {
   static Key<int> Secret;
   struct ContextReadingFS : public ThreadsafeFS {
     IntrusiveRefCntPtr<llvm::vfs::FileSystem>
-    view(llvm::NoneType) const override {
+    viewWithDefaultCWD() const override {
       Got = Context::current().getExisting(Secret);
       return buildTestFS({});
     }
     mutable int Got;
   } FS;
   struct Callbacks : public ClangdServer::Callbacks {
     void onDiagnosticsReady(PathRef File, llvm::StringRef Version,
                             std::vector<Diag> Diagnostics) override {

 // preamble again. (They should be using the preamble's stat-cache)
 TEST(ClangdTests, PreambleVFSStatCache) {
   class StatRecordingFS : public ThreadsafeFS {
   public:
     StatRecordingFS(llvm::StringMap<unsigned> &CountStats)
         : CountStats(CountStats) {}
 
     IntrusiveRefCntPtr<llvm::vfs::FileSystem>
-    view(llvm::NoneType) const override {
+    viewWithDefaultCWD() const override {
       class StatRecordingVFS : public llvm::vfs::ProxyFileSystem {
       public:
         StatRecordingVFS(IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS,
                          llvm::StringMap<unsigned> &CountStats)
             : ProxyFileSystem(std::move(FS)), CountStats(CountStats) {}
 
         llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
         openFileForRead(const Twine &Path) override {

clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp

TEST(HeaderSourceSwitchTest, FileHeuristic) {
   auto FooCpp = testPath("foo.cpp");
   auto FooH = testPath("foo.h");
   auto Invalid = testPath("main.cpp");
 
   FS.Files[FooCpp];
   FS.Files[FooH];
   FS.Files[Invalid];
   Optional<Path> PathResult =
-      getCorrespondingHeaderOrSource(FooCpp, FS.view(llvm::None));
+      getCorrespondingHeaderOrSource(FooCpp, FS.viewWithDefaultCWD());
   EXPECT_TRUE(PathResult.hasValue());
   ASSERT_EQ(PathResult.getValue(), FooH);
 
-  PathResult = getCorrespondingHeaderOrSource(FooH, FS.view(llvm::None));
+  PathResult = getCorrespondingHeaderOrSource(FooH, FS.viewWithDefaultCWD());
   EXPECT_TRUE(PathResult.hasValue());
   ASSERT_EQ(PathResult.getValue(), FooCpp);
 
   // Test with header file in capital letters and different extension, source
   // file with different extension
   auto FooC = testPath("bar.c");
   auto FooHH = testPath("bar.HH");
 
   FS.Files[FooC];
   FS.Files[FooHH];
-  PathResult = getCorrespondingHeaderOrSource(FooC, FS.view(llvm::None));
+  PathResult = getCorrespondingHeaderOrSource(FooC, FS.viewWithDefaultCWD());
   EXPECT_TRUE(PathResult.hasValue());
   ASSERT_EQ(PathResult.getValue(), FooHH);
 
   // Test with both capital letters
   auto Foo2C = testPath("foo2.C");
   auto Foo2HH = testPath("foo2.HH");
   FS.Files[Foo2C];
   FS.Files[Foo2HH];
-  PathResult = getCorrespondingHeaderOrSource(Foo2C, FS.view(llvm::None));
+  PathResult = getCorrespondingHeaderOrSource(Foo2C, FS.viewWithDefaultCWD());
   EXPECT_TRUE(PathResult.hasValue());
   ASSERT_EQ(PathResult.getValue(), Foo2HH);
 
   // Test with source file as capital letter and .hxx header file
   auto Foo3C = testPath("foo3.C");
   auto Foo3HXX = testPath("foo3.hxx");
 
   FS.Files[Foo3C];
   FS.Files[Foo3HXX];
-  PathResult = getCorrespondingHeaderOrSource(Foo3C, FS.view(llvm::None));
+  PathResult = getCorrespondingHeaderOrSource(Foo3C, FS.viewWithDefaultCWD());
   EXPECT_TRUE(PathResult.hasValue());
   ASSERT_EQ(PathResult.getValue(), Foo3HXX);
 
   // Test if asking for a corresponding file that doesn't exist returns an empty
   // string.
-  PathResult = getCorrespondingHeaderOrSource(Invalid, FS.view(llvm::None));
+  PathResult = getCorrespondingHeaderOrSource(Invalid, FS.viewWithDefaultCWD());
   EXPECT_FALSE(PathResult.hasValue());
 }
 
 MATCHER_P(DeclNamed, Name, "") {
   if (const NamedDecl *ND = dyn_cast<NamedDecl>(arg))
     if (ND->getQualifiedNameAsString() == Name)
       return true;
   return false;

clang-tools-extra/clangd/unittests/TestFS.h

 llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
 buildTestFS(llvm::StringMap<std::string> const &Files,
             llvm::StringMap<time_t> const &Timestamps = {});
 
 // A VFS provider that returns TestFSes containing a provided set of files.
 class MockFS : public ThreadsafeFS {
 public:
   IntrusiveRefCntPtr<llvm::vfs::FileSystem>
-  view(llvm::NoneType) const override {
+  viewWithDefaultCWD() const override {
     return buildTestFS(Files, Timestamps);
   }
 
   // If relative paths are used, they are resolved with testPath().
   llvm::StringMap<std::string> Files;
   llvm::StringMap<time_t> Timestamps;
 };