Explicitly mark overriding destructors as "virtual"

It appears that the C++ standard allows overriding destructors to be marked
"override," but at least some MSVC versions complain about it, so at least make
sure such destructors are explicitly marked "virtual."

Change-Id: I0e1cafa7584fd16ebdce61f569eae2373a71b0a1

1 files changed, 19 insertions, 1 deletions

diff --git a/compilerplugins/clang/saloverride.cxx b/compilerplugins/clang/saloverride.cxx
index f37e915ea76e..6b62fb3e0322 100644
--- a/compilerplugins/clang/saloverride.cxx
+++ b/compilerplugins/clang/saloverride.cxx
@@ -40,13 +40,31 @@ bool SalOverride::VisitCXXMethodDecl(CXXMethodDecl const * decl) {
     // external QtCore/qobjectdefs.h:
     if (ignoreLocation(decl) || !compat::isFirstDecl(*decl)
         || decl->begin_overridden_methods() == decl->end_overridden_methods()
-        || decl->hasAttr<OverrideAttr>() || isa<CXXDestructorDecl>(decl)
+        || decl->hasAttr<OverrideAttr>()
         || ignoreLocation(
             compiler.getSourceManager().getSpellingLoc(
                 decl->getNameInfo().getLoc())))
     {
         return true;
     }
+    // It appears that the C++ standard allows overriding destructors to be
+    // marked "override," but at least some MSVC versions complain about it, so
+    // at least make sure such destructors are explicitly marked "virtual":
+    if (isa<CXXDestructorDecl>(decl)) {
+        if (!decl->isVirtualAsWritten()
+            && (rewriter == nullptr
+                || !insertTextBefore(
+                    decl->getSourceRange().getBegin(), "virtual ")))
+        {
+            report(
+                DiagnosticsEngine::Warning,
+                ("overriding destructor declaration not explicitly marked"
+                 " 'virtual'"),
+                decl->getLocation())
+                << decl->getSourceRange();
+        }
+        return true;
+    }
 #if LO_COMPILERPLUGINS_CLANG_COMPAT_HAVE_isAtEndOfImmediateMacroExpansion
     if (rewriter != nullptr) {
         // In  void MACRO(...);  getSourceRange().getEnd() would (erroneously?)