[PATCH] D128157: cppcoreguidelines-virtual-class-destructor: Fix crash when "virtual" keyword is expanded from a macro

[Joachim Priesner via Phabricator via cfe-commits]

jspam created this revision.
Herald added subscribers: carlosgalvezp, shchenz, kbarton, nemanjai.
Herald added a project: All.
jspam requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Check llvm::Optional before dereferencing it.

Compute VirtualEndLoc differently to avoid an assertion failure
in clang::SourceManager::getFileIDLoaded:

  Assertion `0 && "Invalid SLocOffset or bad function choice"' failed


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128157

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===================================================================
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -272,6 +272,7 @@
 } // namespace Bugzilla_51912
 
 namespace macro_tests {
+#define MY_VIRTUAL virtual
 #define CONCAT(x, y) x##y
 
 // CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'FooBar1' is 
protected and virtual [cppcoreguidelines-virtual-class-destructor]
@@ -317,6 +318,15 @@
 protected:
   XMACRO(CONCAT(vir, tual), ~CONCAT(Foo, Bar5());) // no-crash, no-fixit
 };
+
+// CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'FooBar6' is 
protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+1]]:7: note: make it protected and non-virtual
+class FooBar6 {
+protected:
+  MY_VIRTUAL ~FooBar6(); // FIXME: We should have a fixit for this.
+};
+
 #undef XMACRO
 #undef CONCAT
+#undef MY_VIRTUAL
 } // namespace macro_tests
Index: 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===================================================================
--- 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -53,15 +53,17 @@
     return None;
 
   SourceLocation VirtualBeginLoc = Destructor.getBeginLoc();
-  SourceLocation VirtualEndLoc = VirtualBeginLoc.getLocWithOffset(
-      Lexer::MeasureTokenLength(VirtualBeginLoc, SM, LangOpts));
+  SourceLocation VirtualBeginSpellingLoc =
+      SM.getSpellingLoc(Destructor.getBeginLoc());
+  SourceLocation VirtualEndLoc = VirtualBeginSpellingLoc.getLocWithOffset(
+      Lexer::MeasureTokenLength(VirtualBeginSpellingLoc, SM, LangOpts));
 
   /// Range ends with \c StartOfNextToken so that any whitespace after \c
   /// virtual is included.
-  SourceLocation StartOfNextToken =
-      Lexer::findNextToken(VirtualEndLoc, SM, LangOpts)
-          .getValue()
-          .getLocation();
+  Optional<Token> NextToken = Lexer::findNextToken(VirtualEndLoc, SM, 
LangOpts);
+  if (!NextToken)
+    return None;
+  SourceLocation StartOfNextToken = NextToken->getLocation();
 
   return CharSourceRange::getCharRange(VirtualBeginLoc, StartOfNextToken);
 }



Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -272,6 +272,7 @@
 } // namespace Bugzilla_51912
 
 namespace macro_tests {
+#define MY_VIRTUAL virtual
 #define CONCAT(x, y) x##y
 
 // CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'FooBar1' is protected and virtual [cppcoreguidelines-virtual-class-destructor]
@@ -317,6 +318,15 @@
 protected:
   XMACRO(CONCAT(vir, tual), ~CONCAT(Foo, Bar5());) // no-crash, no-fixit
 };
+
+// CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'FooBar6' is protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+1]]:7: note: make it protected and non-virtual
+class FooBar6 {
+protected:
+  MY_VIRTUAL ~FooBar6(); // FIXME: We should have a fixit for this.
+};
+
 #undef XMACRO
 #undef CONCAT
+#undef MY_VIRTUAL
 } // namespace macro_tests
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -53,15 +53,17 @@
     return None;
 
   SourceLocation VirtualBeginLoc = Destructor.getBeginLoc();
-  SourceLocation VirtualEndLoc = VirtualBeginLoc.getLocWithOffset(
-      Lexer::MeasureTokenLength(VirtualBeginLoc, SM, LangOpts));
+  SourceLocation VirtualBeginSpellingLoc =
+      SM.getSpellingLoc(Destructor.getBeginLoc());
+  SourceLocation VirtualEndLoc = VirtualBeginSpellingLoc.getLocWithOffset(
+      Lexer::MeasureTokenLength(VirtualBeginSpellingLoc, SM, LangOpts));
 
   /// Range ends with \c StartOfNextToken so that any whitespace after \c
   /// virtual is included.
-  SourceLocation StartOfNextToken =
-      Lexer::findNextToken(VirtualEndLoc, SM, LangOpts)
-          .getValue()
-          .getLocation();
+  Optional<Token> NextToken = Lexer::findNextToken(VirtualEndLoc, SM, LangOpts);
+  if (!NextToken)
+    return None;
+  SourceLocation StartOfNextToken = NextToken->getLocation();
 
   return CharSourceRange::getCharRange(VirtualBeginLoc, StartOfNextToken);
 }

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits