<div dir="ltr">I'd like a little more test coverage (for the other two type traits that hit this codepath), but otherwise LGTM.<div><br></div><div>It doesn't matter whether we call RequireNonAbstractType or just isAbstract() here, because we don't need to complete the type (we did that a few lines above) and we don't care about its ability to produce nice diagnostics. Using isAbstract() is more consistent with the isIncompleteType() call immediately above, so the code change seems fine as is.</div>
<div><br></div><div>I forget, do you have commit access?</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Apr 14, 2014 at 10:16 AM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
On 14/04/2014 11:59, Nikola Smiljanic wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Please review.<br>
<br>
pr19178.patch<br>
<br>
<br>
diff --git a/lib/Sema/SemaExprCXX.cpp b/lib/Sema/SemaExprCXX.cpp<br>
index 8b9c0e2..2ff501f 100644<br>
--- a/lib/Sema/SemaExprCXX.cpp<br>
+++ b/lib/Sema/SemaExprCXX.cpp<br>
@@ -3656,6 +3656,12 @@ static bool evaluateTypeTrait(Sema &S, TypeTrait Kind, SourceLocation KWLoc,<br>
      if (Args[0]->getType()-><u></u>isIncompleteType())<br>
        return false;<br>
  +    // Make sure the first argument is not an abstract type.<br>
+<br>
+    CXXRecordDecl *RD = Args[0]->getType()-><u></u>getAsCXXRecordDecl();<br>
+    if (RD && RD->isAbstract())<br>
+      return false;<br>
+<br>
</blockquote>
<br>
The fix is correct. Perhaps you could use RequireNonAbstractType() instead of checking for CXXRecordDecl directly, something like this taken from the counterpart in BTT_IsConvertible:<br>
<br>
<br>
-    // Make sure the first argument is a complete type.<br>
-    if (Args[0]->getType()-><u></u>isIncompleteType())<br>
+    // Make sure the first argument is a complete non-abstract type.<br>
+    if (S.RequireCompleteType(KWLoc, Args[0]->getType(), 0) ||<br>
+        S.RequireNonAbstractType(<u></u>KWLoc, Args[0]->getType(), 0))<br>
       return false;<br>
<br>
Either way looks good to me, thanks!<br>
<br>
Alp.<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
      SmallVector<OpaqueValueExpr, 2> OpaqueArgExprs;<br>
      SmallVector<Expr *, 2> ArgExprs;<br>
      ArgExprs.reserve(Args.size() - 1);<br>
diff --git a/test/SemaCXX/type-traits.cpp b/test/SemaCXX/type-traits.cpp<br>
index 28fb8dc..b8557c4 100644<br>
--- a/test/SemaCXX/type-traits.cpp<br>
+++ b/test/SemaCXX/type-traits.cpp<br>
@@ -1964,6 +1964,8 @@ void constructible_checks() {<br>
      { int arr[T(__is_constructible(<u></u>NonPOD, int))]; }<br>
    { int arr[F(__is_nothrow_<u></u>constructible(NonPOD, int))]; }<br>
+<br>
+  { int arr[F(__is_constructible(<u></u>Abstract))]; } // PR19178<br>
  }<br>
    // Instantiation of __is_trivially_constructible<br>
<br>
<br>
______________________________<u></u>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-commits</a><span class="HOEnZb"><font color="#888888"><br>
</font></span></blockquote><span class="HOEnZb"><font color="#888888">
<br>
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
______________________________<u></u>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-commits</a><br>
</font></span></blockquote></div><br></div>