[PATCH] Refactor VariantMatcher to use a tagged union.

Samuel Benzaquen sbenza at google.com
Tue Aug 20 08:46:07 PDT 2013



================
Comment at: lib/ASTMatchers/Dynamic/VariantValue.cpp:160
@@ -84,1 +159,3 @@
+      if (CanConstructCallback(*AsPolymorphic->Matchers[i]))
+        return AsPolymorphic->Matchers[i];
     }
----------------
Stefanus Du Toit wrote:
> This changes the behaviour of this function - previously if more than one matcher passed the test it would return null, now it returns the first one. I don't know if you intended that, but if you did, you should update the comment at the function's declaration.
There should be only one, because the caller already checked (asserts on hasTypedMatcher()).
Replaced the return NULL with llvm_unreachable, since it is; and updated the comment in the header.

================
Comment at: lib/ASTMatchers/Dynamic/VariantValue.cpp:156
@@ +155,3 @@
+  case RT_Nothing: return NULL;
+  case RT_Single: return AsSingle->Matcher.get();
+  case RT_Polymorphic:
----------------
Stefanus Du Toit wrote:
> Shouldn't this still call CanConstructCallback()?
The caller is asserting that hasTypedMatcher<T>() is true. This is because the user must check hasTypedMatcher first, and then call getTypedMatcher.
I added another assertion here for documentation.


http://llvm-reviews.chandlerc.com/D1446



More information about the cfe-commits mailing list