[clangd] Suppress GCC -Woverloaded-virtual by renaming ThreadsafeFS extension point

[sammccall]

Summary

By making all overloads non-virtual and delegating to a differently-named
private method, we avoid any (harmless) name-hiding in the subclasses.

[kadircet]

thanks, LGTM!

clang-tools-extra/clangd/Preamble.cpp

231–232

private

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

49–50

make this private ?

nit: also make the occurences in tests private

clang-tools-extra/clangd/Preamble.cpp

 };
 
 /// Scans the preprocessor directives in the preamble section of the file by
 /// 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 {
+  private:
+    llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> viewImpl() 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;

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

 /// snapshot semantics. clangd will not create vfs::FileSystems for use in
 /// 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;
+  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
+  view(llvm::NoneType CWD) const {
+    return viewImpl();
+  }
 
   /// 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>
-  view(PathRef CWD) const;
+  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> view(PathRef CWD) const;
+
+private:
+  /// Overridden by implementations to provide a vfs::FileSystem.
+  /// This is distinct from view(NoneType) to avoid GCC's -Woverloaded-virtual.
+  virtual llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> viewImpl() const = 0;
 };
 
 class RealThreadsafeFS : public ThreadsafeFS {
-public:
-  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
-      view(llvm::NoneType) const override;
+private:
+  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> viewImpl() const override;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

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

 ThreadsafeFS::view(PathRef CWD) const {
   auto FS = view(llvm::None);
   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::viewImpl() 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/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 {
+    mutable int Got;
+
+  private:
+    IntrusiveRefCntPtr<llvm::vfs::FileSystem> viewImpl() 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 {
       Got = Context::current().getExisting(Secret);
     }
     int Got;
   } Callbacks;

 }
 
 // FIXME(ioeric): make this work for windows again.
 #ifndef _WIN32
 // Check that running code completion doesn't stat() a bunch of files from the
 // preamble again. (They should be using the preamble's stat-cache)
 TEST(ClangdTests, PreambleVFSStatCache) {
   class StatRecordingFS : public ThreadsafeFS {
+    llvm::StringMap<unsigned> &CountStats;
+
   public:
+    // If relative paths are used, they are resolved with testPath().
+    llvm::StringMap<std::string> Files;
+
     StatRecordingFS(llvm::StringMap<unsigned> &CountStats)
         : CountStats(CountStats) {}
 
-    IntrusiveRefCntPtr<llvm::vfs::FileSystem>
-    view(llvm::NoneType) const override {
+  private:
+    IntrusiveRefCntPtr<llvm::vfs::FileSystem> viewImpl() 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 {
           ++CountStats[llvm::sys::path::filename(Path.str())];
           return ProxyFileSystem::openFileForRead(Path);
         }
         llvm::ErrorOr<llvm::vfs::Status> status(const Twine &Path) override {
           ++CountStats[llvm::sys::path::filename(Path.str())];
           return ProxyFileSystem::status(Path);
         }
 
       private:
         llvm::StringMap<unsigned> &CountStats;
       };
 
       return IntrusiveRefCntPtr<StatRecordingVFS>(
           new StatRecordingVFS(buildTestFS(Files), CountStats));
     }
-
-    // If relative paths are used, they are resolved with testPath().
-    llvm::StringMap<std::string> Files;
-    llvm::StringMap<unsigned> &CountStats;
   };
 
   llvm::StringMap<unsigned> CountStats;
   StatRecordingFS FS(CountStats);
   ErrorCheckingCallbacks DiagConsumer;
   MockCompilationDatabase CDB;
   ClangdServer Server(CDB, FS, ClangdServer::optsForTest(), &DiagConsumer);
 

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

 // directories.
 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 {
+  IntrusiveRefCntPtr<llvm::vfs::FileSystem> viewImpl() 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;
 };