Re: [PATCH] D23765: Fix for clang PR 29087

[Taewook Oh via cfe-commits]

twoh added a comment.

ping


https://reviews.llvm.org/D23765



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

Re: [PATCH] D23765: Fix for clang PR 29087

[Taewook Oh via cfe-commits]

twoh added a comment.

@rsmith Thank you for your review! I added tests to cxx11-crashes.cpp, as the 
goal of this patch is not handling __has_* traits right but preventing ICE. 
Also, I tried to use ConstructorUsingShadowDecl::getConstructor instead of 
ConstructorUsingShadowDecl::getTargetDecl followed by casting, but clang build 
was failed with undefined references. I wonder if the definition of 
ConstructorUsingShadowDecl::getConstructor is missed in 
https://reviews.llvm.org/rL274049.


https://reviews.llvm.org/D23765



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

Re: [PATCH] D23765: Fix for clang PR 29087

[Taewook Oh via cfe-commits]

twoh updated this revision to Diff 71711.
twoh added a comment.

Updated diff. For ConstructorUsingShadowDecl, test with its target 
CXXConstructorDecl, but only when it is not a default/copy/move constructor.


https://reviews.llvm.org/D23765

Files:
  lib/Sema/SemaExprCXX.cpp
  test/SemaCXX/cxx11-crashes.cpp

Index: test/SemaCXX/cxx11-crashes.cpp
===
--- test/SemaCXX/cxx11-crashes.cpp
+++ test/SemaCXX/cxx11-crashes.cpp
@@ -91,3 +91,10 @@
   Foo(lambda);
 }
 }
+
+namespace pr29091 {
+  struct X{ X(const X ); };
+  struct Y: X { using X::X; };
+  bool foo() { return __has_nothrow_constructor(Y); }
+  bool bar() { return __has_nothrow_copy(Y); }
+}
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -292,7 +292,7 @@
   if (isDependent) {
 // We didn't find our type, but that's okay: it's dependent
 // anyway.
-
+
 // FIXME: What if we have no nested-name-specifier?
 QualType T = CheckTypenameType(ETK_None, SourceLocation(),
SS.getWithLocInContext(Context),
@@ -326,14 +326,14 @@
 ParsedType Sema::getDestructorType(const DeclSpec& DS, ParsedType ObjectType) {
 if (DS.getTypeSpecType() == DeclSpec::TST_error || !ObjectType)
   return nullptr;
-assert(DS.getTypeSpecType() == DeclSpec::TST_decltype 
+assert(DS.getTypeSpecType() == DeclSpec::TST_decltype
&& "only get destructor types from declspecs");
 QualType T = BuildDecltypeType(DS.getRepAsExpr(), DS.getTypeSpecTypeLoc());
 QualType SearchType = GetTypeFromParser(ObjectType);
 if (SearchType->isDependentType() || Context.hasSameUnqualifiedType(SearchType, T)) {
   return ParsedType::make(T);
 }
-  
+
 Diag(DS.getTypeSpecTypeLoc(), diag::err_destructor_expr_type_mismatch)
   << T << SearchType;
 return nullptr;
@@ -662,7 +662,7 @@
   IsThrownVarInScope = true;
   break;
 }
-
+
 if (S->getFlags() &
 (Scope::FnScope | Scope::ClassScope | Scope::BlockScope |
  Scope::FunctionPrototypeScope | Scope::ObjCMethodScope |
@@ -672,11 +672,11 @@
 }
   }
   }
-  
+
   return BuildCXXThrow(OpLoc, Ex, IsThrownVarInScope);
 }
 
-ExprResult Sema::BuildCXXThrow(SourceLocation OpLoc, Expr *Ex, 
+ExprResult Sema::BuildCXXThrow(SourceLocation OpLoc, Expr *Ex,
bool IsThrownVarInScope) {
   // Don't report an error if 'throw' is used in system headers.
   if (!getLangOpts().CXXExceptions &&
@@ -903,10 +903,10 @@
I-- && isa(FunctionScopes[I]);
CurDC = getLambdaAwareParentOfDeclContext(CurDC)) {
 CurLSI = cast(FunctionScopes[I]);
-
-if (!CurLSI->isCXXThisCaptured()) 
+
+if (!CurLSI->isCXXThisCaptured())
 continue;
-  
+
 auto C = CurLSI->getCXXThisCapture();
 
 if (C.isCopyCapture()) {
@@ -922,7 +922,7 @@
 assert(CurLSI);
 assert(isGenericLambdaCallOperatorSpecialization(CurLSI->CallOperator));
 assert(CurDC == getLambdaAwareParentOfDeclContext(CurLSI->CallOperator));
-
+
 auto IsThisCaptured =
 [](CXXRecordDecl *Closure, bool , bool ) {
   IsConst = false;
@@ -992,10 +992,10 @@
   return ThisTy;
 }
 
-Sema::CXXThisScopeRAII::CXXThisScopeRAII(Sema , 
+Sema::CXXThisScopeRAII::CXXThisScopeRAII(Sema ,
  Decl *ContextDecl,
  unsigned CXXThisTypeQuals,
- bool Enabled) 
+ bool Enabled)
   : S(S), OldCXXThisTypeOverride(S.CXXThisTypeOverride), Enabled(false)
 {
   if (!Enabled || !ContextDecl)
@@ -1006,13 +1006,13 @@
 Record = Template->getTemplatedDecl();
   else
 Record = cast(ContextDecl);
-
+
   // We care only for CVR qualifiers here, so cut everything else.
   CXXThisTypeQuals &= Qualifiers::FastMask;
   S.CXXThisTypeOverride
 = S.Context.getPointerType(
 S.Context.getRecordType(Record).withCVRQualifiers(CXXThisTypeQuals));
-  
+
   this->Enabled = true;
 }
 
@@ -1026,7 +1026,7 @@
 static Expr *captureThis(Sema , ASTContext , RecordDecl *RD,
  QualType ThisTy, SourceLocation Loc,
  const bool ByCopy) {
- 
+
   QualType AdjustedThisTy = ThisTy;
   // The type of the corresponding data member (not a 'this' pointer if 'by
   // copy').
@@ -1039,7 +1039,7 @@
 CaptureThisFieldTy.removeLocalCVRQualifiers(Qualifiers::CVRMask);
 AdjustedThisTy = Context.getPointerType(CaptureThisFieldTy);
   }
-  
+
   FieldDecl *Field = FieldDecl::Create(
   Context, RD, Loc, Loc, nullptr, CaptureThisFieldTy,
   Context.getTrivialTypeSourceInfo(CaptureThisFieldTy, Loc), nullptr, false,
@@ -1065,41 +1065,41 @@
   return This;
 }
 
-bool 

Re: [PATCH] D23765: Fix for clang PR 29087

[Richard Smith via cfe-commits]

rsmith added inline comments.


Comment at: lib/Sema/SemaExprCXX.cpp:4227
@@ -4226,1 +4226,3 @@
   continue;
+// Using(Shadow)Decl itself is not a constructor
+if (isa(ND) || isa(ND))

This isn't really right: a `UsingShadowDecl` whose underlying declaration is a 
constructor should itself act like a constructor. This could matter in some 
obscure cases:

  struct B;
  struct A { A(B&); };
  struct B : A { using A::A; };

What does `__has_nothrow_copy(B)` return? It should probably be `false`, since 
copying a non-const `B` object will invoke the `A(B&)` constructor, which may 
throw, even though the `B(const B&)` constructor does not.

However, these `__has_*` traits should be considered deprecated and are 
essentially useless, so perhaps it doesn't matter too much whether we get these 
corner cases right. We also get the constructor template case "wrong" here, 
which I would imagine comes up a lot more frequently.


Comment at: test/SemaCXX/crash-has-nothrow-constructor.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s
+

Please add these tests into some existing test file for the type traits rather 
than adding two new files.


https://reviews.llvm.org/D23765



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

Re: [PATCH] D23765: Fix for clang PR 29087

[Taewook Oh via cfe-commits]

twoh updated this revision to Diff 71560.
twoh added a comment.

Tests added


https://reviews.llvm.org/D23765

Files:
  lib/Sema/SemaExprCXX.cpp
  test/SemaCXX/crash-has-nothrow-constructor.cpp
  test/SemaCXX/crash-has-nothrow-copy.cpp

Index: test/SemaCXX/crash-has-nothrow-copy.cpp
===
--- test/SemaCXX/crash-has-nothrow-copy.cpp
+++ test/SemaCXX/crash-has-nothrow-copy.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s
+
+#define assert(x) if (x) {}
+
+struct X {
+  X(const X ) {}
+};
+
+struct Y : public X {
+public:
+  using X::X;
+};
+
+int main() {
+  assert(__has_nothrow_copy(Y));
+}
Index: test/SemaCXX/crash-has-nothrow-constructor.cpp
===
--- test/SemaCXX/crash-has-nothrow-constructor.cpp
+++ test/SemaCXX/crash-has-nothrow-constructor.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s
+
+#define assert(x) if (x) {}
+
+struct X {
+  X() {}
+};
+
+struct Y : public X {
+public:
+  using X::X;
+};
+
+int main() {
+  assert(__has_nothrow_constructor(Y));
+}
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -292,7 +292,7 @@
   if (isDependent) {
 // We didn't find our type, but that's okay: it's dependent
 // anyway.
-
+
 // FIXME: What if we have no nested-name-specifier?
 QualType T = CheckTypenameType(ETK_None, SourceLocation(),
SS.getWithLocInContext(Context),
@@ -326,14 +326,14 @@
 ParsedType Sema::getDestructorType(const DeclSpec& DS, ParsedType ObjectType) {
 if (DS.getTypeSpecType() == DeclSpec::TST_error || !ObjectType)
   return nullptr;
-assert(DS.getTypeSpecType() == DeclSpec::TST_decltype 
+assert(DS.getTypeSpecType() == DeclSpec::TST_decltype
&& "only get destructor types from declspecs");
 QualType T = BuildDecltypeType(DS.getRepAsExpr(), DS.getTypeSpecTypeLoc());
 QualType SearchType = GetTypeFromParser(ObjectType);
 if (SearchType->isDependentType() || Context.hasSameUnqualifiedType(SearchType, T)) {
   return ParsedType::make(T);
 }
-  
+
 Diag(DS.getTypeSpecTypeLoc(), diag::err_destructor_expr_type_mismatch)
   << T << SearchType;
 return nullptr;
@@ -662,7 +662,7 @@
   IsThrownVarInScope = true;
   break;
 }
-
+
 if (S->getFlags() &
 (Scope::FnScope | Scope::ClassScope | Scope::BlockScope |
  Scope::FunctionPrototypeScope | Scope::ObjCMethodScope |
@@ -672,11 +672,11 @@
 }
   }
   }
-  
+
   return BuildCXXThrow(OpLoc, Ex, IsThrownVarInScope);
 }
 
-ExprResult Sema::BuildCXXThrow(SourceLocation OpLoc, Expr *Ex, 
+ExprResult Sema::BuildCXXThrow(SourceLocation OpLoc, Expr *Ex,
bool IsThrownVarInScope) {
   // Don't report an error if 'throw' is used in system headers.
   if (!getLangOpts().CXXExceptions &&
@@ -903,10 +903,10 @@
I-- && isa(FunctionScopes[I]);
CurDC = getLambdaAwareParentOfDeclContext(CurDC)) {
 CurLSI = cast(FunctionScopes[I]);
-
-if (!CurLSI->isCXXThisCaptured()) 
+
+if (!CurLSI->isCXXThisCaptured())
 continue;
-  
+
 auto C = CurLSI->getCXXThisCapture();
 
 if (C.isCopyCapture()) {
@@ -922,7 +922,7 @@
 assert(CurLSI);
 assert(isGenericLambdaCallOperatorSpecialization(CurLSI->CallOperator));
 assert(CurDC == getLambdaAwareParentOfDeclContext(CurLSI->CallOperator));
-
+
 auto IsThisCaptured =
 [](CXXRecordDecl *Closure, bool , bool ) {
   IsConst = false;
@@ -992,10 +992,10 @@
   return ThisTy;
 }
 
-Sema::CXXThisScopeRAII::CXXThisScopeRAII(Sema , 
+Sema::CXXThisScopeRAII::CXXThisScopeRAII(Sema ,
  Decl *ContextDecl,
  unsigned CXXThisTypeQuals,
- bool Enabled) 
+ bool Enabled)
   : S(S), OldCXXThisTypeOverride(S.CXXThisTypeOverride), Enabled(false)
 {
   if (!Enabled || !ContextDecl)
@@ -1006,13 +1006,13 @@
 Record = Template->getTemplatedDecl();
   else
 Record = cast(ContextDecl);
-
+
   // We care only for CVR qualifiers here, so cut everything else.
   CXXThisTypeQuals &= Qualifiers::FastMask;
   S.CXXThisTypeOverride
 = S.Context.getPointerType(
 S.Context.getRecordType(Record).withCVRQualifiers(CXXThisTypeQuals));
-  
+
   this->Enabled = true;
 }
 
@@ -1026,7 +1026,7 @@
 static Expr *captureThis(Sema , ASTContext , RecordDecl *RD,
  QualType ThisTy, SourceLocation Loc,
  const bool ByCopy) {
- 
+
   QualType AdjustedThisTy = ThisTy;
   // The type of the corresponding data member (not a 'this' pointer if 'by
   // copy').
@@ 

Re: [PATCH] D23765: Fix for clang PR 29087

[Serge Pavlov via cfe-commits]

sepavloff added a subscriber: sepavloff.
sepavloff added a comment.

You need to provide a test for the fix.


https://reviews.llvm.org/D23765



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

Re: [PATCH] D23765: Fix for clang PR 29087

[Taewook Oh via cfe-commits]

twoh added a comment.

Ping.


https://reviews.llvm.org/D23765



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

Re: [PATCH] D23765: Fix for clang PR 29087

[Taewook Oh via cfe-commits]

twoh added a comment.

ping


https://reviews.llvm.org/D23765



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