[clang-tools-extra] 7256898 - [clangd] Suppress GCC -Woverloaded-virtual by renaming ThreadsafeFS extension point

[Sam McCall via cfe-commits]

Author: Sam McCall
Date: 2020-06-30T15:06:15+02:00
New Revision: 72568984b8040fa8de70a6b4f9c63b6a5aac8d3f

URL: https://github.com/llvm/llvm-project/commit/72568984b8040fa8de70a6b4f9c63b6a5aac8d3f
DIFF: https://github.com/llvm/llvm-project/commit/72568984b8040fa8de70a6b4f9c63b6a5aac8d3f.diff

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

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

Reviewers: kadircet

Subscribers: kristof.beyls, ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits, Quuxplusone, dblaikie

Tags: #clang

Differential Revision: https://reviews.llvm.org/D82793

Added: 
    

Modified: 
    clang-tools-extra/clangd/Preamble.cpp
    clang-tools-extra/clangd/support/ThreadsafeFS.cpp
    clang-tools-extra/clangd/support/ThreadsafeFS.h
    clang-tools-extra/clangd/unittests/ClangdTests.cpp
    clang-tools-extra/clangd/unittests/TestFS.h

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index ca0a76db78f4..b71afa0b1619 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -229,9 +229,8 @@ struct ScannedPreamble {
 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;
     }
   };

diff  --git a/clang-tools-extra/clangd/support/ThreadsafeFS.cpp b/clang-tools-extra/clangd/support/ThreadsafeFS.cpp
index a25ac773bfa2..cadda8efa095 100644
--- a/clang-tools-extra/clangd/support/ThreadsafeFS.cpp
+++ b/clang-tools-extra/clangd/support/ThreadsafeFS.cpp
@@ -83,7 +83,7 @@ ThreadsafeFS::view(PathRef CWD) const {
 }
 
 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.

diff  --git a/clang-tools-extra/clangd/support/ThreadsafeFS.h b/clang-tools-extra/clangd/support/ThreadsafeFS.h
index aa6825fb3999..ad0dd6425811 100644
--- a/clang-tools-extra/clangd/support/ThreadsafeFS.h
+++ b/clang-tools-extra/clangd/support/ThreadsafeFS.h
@@ -30,20 +30,25 @@ class ThreadsafeFS {
   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

diff  --git a/clang-tools-extra/clangd/unittests/ClangdTests.cpp b/clang-tools-extra/clangd/unittests/ClangdTests.cpp
index e1f02c897fe3..04e97d0fa3f9 100644
--- a/clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -273,12 +273,13 @@ int b = a;
 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,
@@ -925,12 +926,17 @@ TEST_F(ClangdVFSTest, ChangedHeaderFromISystem) {
 // 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,
@@ -954,10 +960,6 @@ TEST(ClangdTests, PreambleVFSStatCache) {
       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;

diff  --git a/clang-tools-extra/clangd/unittests/TestFS.h b/clang-tools-extra/clangd/unittests/TestFS.h
index a487a361bfe1..1c713c78f7bf 100644
--- a/clang-tools-extra/clangd/unittests/TestFS.h
+++ b/clang-tools-extra/clangd/unittests/TestFS.h
@@ -33,8 +33,7 @@ buildTestFS(llvm::StringMap<std::string> const &Files,
 // 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);
   }