[PATCH] D62435: Add Attribute NoThrow as an Exception Specifier Type

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

This change had another breaking effect as well, for the MinGW target. Code 
that implements a COM interface easily ends up overriding a 
`__declspec(nothrow)` function with a method that in most cases (at least in 
cases that follow the microsoft sample code) lacks the same attribute. For code 
with `-fms-extensions`, this is a warning (as all exception spec mismatches are 
non-fatal there), but MinGW code doesn't necessarily use this option. See 
https://bugs.llvm.org/show_bug.cgi?id=42100 for full issue report and 
discussion.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62435/new/

https://reviews.llvm.org/D62435



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

[PATCH] D62435: Add Attribute NoThrow as an Exception Specifier Type

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D62435#1524824 , @mstorsjo wrote:

> This change broke compiling Qt on MinGW, see 
> https://bugs.llvm.org/show_bug.cgi?id=42089. Trivially reproducible by trying 
> to compile a snippet that looks like this:
>
>   class Foo {
>   public:
>   __attribute__((nothrow)) void __attribute__((__stdcall__)) Bar();
>   };
>  
>   void Foo::Bar() {
>   }
>
>
> Reproducible by compiling the same snippet for a windows-msvc target as well.


Yikes, thanks for the report!  I'll look at it this morning.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62435/new/

https://reviews.llvm.org/D62435



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

[PATCH] D62435: Add Attribute NoThrow as an Exception Specifier Type

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

This change broke compiling Qt on MinGW, see 
https://bugs.llvm.org/show_bug.cgi?id=42089. Trivially reproducible by trying 
to compile a snippet that looks like this:

  class Foo {
  public:
  __attribute__((nothrow)) void __attribute__((__stdcall__)) Bar();
  };
  
  void Foo::Bar() {
  }

Reproducible by compiling the same snippet for a windows-msvc target as well.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62435/new/

https://reviews.llvm.org/D62435



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

[PATCH] D62435: Add Attribute NoThrow as an Exception Specifier Type

[Erich Keane via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
erichkeane marked 2 inline comments as done.
Closed by commit rL362119: Add Attribute NoThrow as an Exception Specifier Type 
(authored by erichkeane, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D62435?vs=201673=202249#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62435/new/

https://reviews.llvm.org/D62435

Files:
  cfe/trunk/include/clang-c/Index.h
  cfe/trunk/include/clang/AST/Decl.h
  cfe/trunk/include/clang/AST/Type.h
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/include/clang/Basic/ExceptionSpecificationType.h
  cfe/trunk/lib/AST/ASTContext.cpp
  cfe/trunk/lib/AST/JSONNodeDumper.cpp
  cfe/trunk/lib/AST/Type.cpp
  cfe/trunk/lib/Sema/SemaDeclAttr.cpp
  cfe/trunk/lib/Sema/SemaDeclCXX.cpp
  cfe/trunk/lib/Sema/SemaExprCXX.cpp
  cfe/trunk/lib/Sema/SemaType.cpp
  cfe/trunk/test/SemaCXX/nothrow-vs-exception-specs.cpp
  cfe/trunk/tools/libclang/CXType.cpp

Index: cfe/trunk/lib/AST/Type.cpp
===
--- cfe/trunk/lib/AST/Type.cpp
+++ cfe/trunk/lib/AST/Type.cpp
@@ -3077,6 +3077,7 @@
   case EST_DynamicNone:
   case EST_BasicNoexcept:
   case EST_NoexceptTrue:
+  case EST_NoThrow:
 return CT_Cannot;
 
   case EST_None:
Index: cfe/trunk/lib/AST/JSONNodeDumper.cpp
===
--- cfe/trunk/lib/AST/JSONNodeDumper.cpp
+++ cfe/trunk/lib/AST/JSONNodeDumper.cpp
@@ -464,7 +464,9 @@
 //JOS.attributeWithCall("exceptionSpecExpr",
 //[this, E]() { Visit(E.ExceptionSpec.NoexceptExpr); });
 break;
-
+  case EST_NoThrow:
+JOS.attribute("exceptionSpec", "nothrow");
+break;
   // FIXME: I cannot find a way to trigger these cases while dumping the AST. I
   // suspect you can only run into them when executing an AST dump from within
   // the debugger, which is not a use case we worry about for the JSON dumping
Index: cfe/trunk/lib/AST/ASTContext.cpp
===
--- cfe/trunk/lib/AST/ASTContext.cpp
+++ cfe/trunk/lib/AST/ASTContext.cpp
@@ -3742,7 +3742,10 @@
 break;
   }
 
-  case EST_DynamicNone: case EST_BasicNoexcept: case EST_NoexceptTrue:
+  case EST_DynamicNone:
+  case EST_BasicNoexcept:
+  case EST_NoexceptTrue:
+  case EST_NoThrow:
 CanonicalEPI.ExceptionSpec.Type = EST_BasicNoexcept;
 break;
 
Index: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
===
--- cfe/trunk/lib/Sema/SemaDeclAttr.cpp
+++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp
@@ -6853,7 +6853,8 @@
 handleNoCfCheckAttr(S, D, AL);
 break;
   case ParsedAttr::AT_NoThrow:
-handleSimpleAttribute(S, D, AL);
+if (!AL.isUsedAsTypeAttr())
+  handleSimpleAttribute(S, D, AL);
 break;
   case ParsedAttr::AT_CUDAShared:
 handleSharedAttr(S, D, AL);
Index: cfe/trunk/lib/Sema/SemaExprCXX.cpp
===
--- cfe/trunk/lib/Sema/SemaExprCXX.cpp
+++ cfe/trunk/lib/Sema/SemaExprCXX.cpp
@@ -6045,6 +6045,8 @@
   if (EST2 == EST_NoexceptFalse) return ESI2;
 
   // If either of them is non-throwing, the result is the other.
+  if (EST1 == EST_NoThrow) return ESI2;
+  if (EST2 == EST_NoThrow) return ESI1;
   if (EST1 == EST_DynamicNone) return ESI2;
   if (EST2 == EST_DynamicNone) return ESI1;
   if (EST1 == EST_BasicNoexcept) return ESI2;
@@ -6073,6 +6075,7 @@
   case EST_DependentNoexcept:
   case EST_NoexceptFalse:
   case EST_NoexceptTrue:
+  case EST_NoThrow:
 llvm_unreachable("handled above");
 
   case EST_Dynamic: {
Index: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
===
--- cfe/trunk/lib/Sema/SemaDeclCXX.cpp
+++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp
@@ -192,6 +192,7 @@
   // If this function has a basic noexcept, it doesn't affect the outcome.
   case EST_BasicNoexcept:
   case EST_NoexceptTrue:
+  case EST_NoThrow:
 return;
   // If we're still at noexcept(true) and there's a throw() callee,
   // change to that specification.
@@ -15457,6 +15458,7 @@
   case EST_Uninstantiated:
   case EST_Unevaluated:
   case EST_BasicNoexcept:
+  case EST_NoThrow:
   case EST_DynamicNone:
   case EST_MSAny:
   case EST_None:
Index: cfe/trunk/lib/Sema/SemaType.cpp
===
--- cfe/trunk/lib/Sema/SemaType.cpp
+++ cfe/trunk/lib/Sema/SemaType.cpp
@@ -130,6 +130,7 @@
   case ParsedAttr::AT_Regparm: \
   case ParsedAttr::AT_AnyX86NoCallerSavedRegisters:\
   case ParsedAttr::AT_AnyX86NoCfCheck: \
+  case ParsedAttr::AT_NoThrow:  

[PATCH] D62435: Add Attribute NoThrow as an Exception Specifier Type

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 6 inline comments as done.
erichkeane added inline comments.



Comment at: clang/include/clang/Sema/DeclSpec.h:1547
 
+  bool hasAttr(ParsedAttr::Kind Kind) const {
+return llvm::find_if(getAttrs(), [Kind](const ParsedAttr ) {

aaron.ballman wrote:
> Not that I dislike this, but is this function being used? It seems to be the 
> only `hasAttr` in the review.
Woops, likely a leftover from a previous iteration, removing it!



Comment at: clang/lib/Sema/SemaType.cpp:6971
+  switch (Proto->getExceptionSpecType()) {
+  case EST_None: llvm_unreachable("This doesn't have an exception spec!");
+  case EST_DynamicNone:

aaron.ballman wrote:
> Will this need a fallthrough attribute because of the statement between the 
> labels?
Ah, I think it depends on what llvm_unreachable ends up expanding to (actually, 
LLVM_ATTRIBUTE_NORETURN). 

Basically, here: https://llvm.org/doxygen/Support_2ErrorHandling_8h_source.html

I'm not sure of what compilers has that expand to nothing don't support it 
(would need !def __GNUC__ and !def _MSC_VER), but an LLVM_FALLTHROUGH is easy 
enough to add, thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62435/new/

https://reviews.llvm.org/D62435



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

[PATCH] D62435: Add Attribute NoThrow as an Exception Specifier Type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM aside from some minor nits.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2789
   InGroup;
+def warn_nothrow_attribute_ignored : Warning<"nothrow attribute conflicts with"
+  " exception specification; attribute ignored">,

Add single quotes around the attribute name: `'nothrow' attribute conflicts...`



Comment at: clang/include/clang/Sema/DeclSpec.h:1547
 
+  bool hasAttr(ParsedAttr::Kind Kind) const {
+return llvm::find_if(getAttrs(), [Kind](const ParsedAttr ) {

Not that I dislike this, but is this function being used? It seems to be the 
only `hasAttr` in the review.



Comment at: clang/lib/Sema/SemaType.cpp:6968
+
+// MSVC ignores nothrow for exception specification if it is in conflict.
+if (Proto->hasExceptionSpec()) {

How about: `MSVC ignores nothrow if it is in conflict with an explicit 
exception specification.`



Comment at: clang/lib/Sema/SemaType.cpp:6971
+  switch (Proto->getExceptionSpecType()) {
+  case EST_None: llvm_unreachable("This doesn't have an exception spec!");
+  case EST_DynamicNone:

Will this need a fallthrough attribute because of the statement between the 
labels?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62435/new/

https://reviews.llvm.org/D62435



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

[PATCH] D62435: Add Attribute NoThrow as an Exception Specifier Type

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Seems fine to me; please wait for @aaron.ballman's review to conclude as well.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62435/new/

https://reviews.llvm.org/D62435



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

[PATCH] D62435: Add Attribute NoThrow as an Exception Specifier Type

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 201673.
erichkeane marked 5 inline comments as done.
erichkeane added a comment.

Added warning + other comments from @aaron.ballman

The exception state was further along than I expected, so I was able to make 
the warning better than I thought!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62435/new/

https://reviews.llvm.org/D62435

Files:
  clang/include/clang-c/Index.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/ExceptionSpecificationType.h
  clang/include/clang/Sema/DeclSpec.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/Type.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/SemaCXX/nothrow-vs-exception-specs.cpp
  clang/tools/libclang/CXType.cpp

Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -742,6 +742,8 @@
 return CXCursor_ExceptionSpecificationKind_MSAny;
   case EST_BasicNoexcept:
 return CXCursor_ExceptionSpecificationKind_BasicNoexcept;
+  case EST_NoThrow:
+return CXCursor_ExceptionSpecificationKind_NoThrow;
   case EST_NoexceptFalse:
   case EST_NoexceptTrue:
   case EST_DependentNoexcept:
Index: clang/test/SemaCXX/nothrow-vs-exception-specs.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/nothrow-vs-exception-specs.cpp
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 %s -fcxx-exceptions -fdeclspec -fsyntax-only -Wexceptions -verify -std=c++14
+// RUN: %clang_cc1 %s -fcxx-exceptions -fdeclspec -fsyntax-only -Wexceptions -verify -std=c++17 -DCPP17
+
+__attribute__((nothrow)) void f1();
+static_assert(noexcept(f1()), "");
+void f1() noexcept;
+// expected-error@+2 {{exception specification in declaration does not match previous declaration}}
+// expected-note@-2 {{previous declaration is here}}
+void f1() noexcept(false);
+
+__attribute__((nothrow)) void f2();
+static_assert(noexcept(f2()), "");
+// expected-error@+2 {{exception specification in declaration does not match previous declaration}}
+// expected-note@-3 {{previous declaration is here}}
+void f2() noexcept(false);
+
+void f3() __attribute__((nothrow));
+static_assert(noexcept(f3()), "");
+void f3() noexcept;
+// expected-error@+2 {{exception specification in declaration does not match previous declaration}}
+// expected-note@-2 {{previous declaration is here}}
+void f3() noexcept(false);
+
+// Still noexcept due to throw()
+__attribute__((nothrow)) void f4() throw();
+static_assert(noexcept(f4()), "");
+
+// Still noexcept due to noexcept
+__attribute__((nothrow)) void f5() noexcept;
+static_assert(noexcept(f5()), "");
+
+// Still noexcept due to noexcept(true)
+__attribute__((nothrow)) void f6() noexcept(true);
+static_assert(noexcept(f6()), "");
+
+#ifndef CPP17
+// Doesn't override C++ implementation.
+// expected-warning@+1{{nothrow attribute conflicts with exception specification; attribute ignored}}
+__attribute__((nothrow)) void f7() throw(int);
+static_assert(!noexcept(f7()), "");
+#endif
+
+// Doesn't override C++ implementation.
+// expected-warning@+1{{nothrow attribute conflicts with exception specification; attribute ignored}}
+__attribute__((nothrow)) void f8() noexcept(false);
+static_assert(!noexcept(f8()), "");
+
+__declspec(nothrow) void foo1() noexcept;
+__declspec(nothrow) void foo2() noexcept(true);
+// expected-warning@+1{{nothrow attribute conflicts with exception specification; attribute ignored}}
+__declspec(nothrow) void foo3() noexcept(false);
+__declspec(nothrow) void foo4() noexcept(noexcept(foo1()));
+__declspec(nothrow) void foo5() noexcept(noexcept(foo2()));
+// expected-warning@+1{{nothrow attribute conflicts with exception specification; attribute ignored}}
+__declspec(nothrow) void foo6() noexcept(noexcept(foo3()));
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -130,6 +130,7 @@
   case ParsedAttr::AT_Regparm: \
   case ParsedAttr::AT_AnyX86NoCallerSavedRegisters:\
   case ParsedAttr::AT_AnyX86NoCfCheck: \
+  case ParsedAttr::AT_NoThrow: \
 CALLING_CONV_ATTRS_CASELIST
 
 // Microsoft-specific type qualifiers.
@@ -4516,7 +4517,7 @@
   // If the function declarator has a prototype (i.e. it is not () and
   // does not have a K identifier list), then the arguments are part
   // of the type, otherwise the argument list is ().
-  const DeclaratorChunk::FunctionTypeInfo  = DeclType.Fun;
+  DeclaratorChunk::FunctionTypeInfo  

[PATCH] D62435: Add Attribute NoThrow as an Exception Specifier Type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaType.cpp:6970
+if (Proto->hasExceptionSpec())
+  return true;
+

erichkeane wrote:
> aaron.ballman wrote:
> > I think we should diagnose that the `nothrow` is ignored in this case to 
> > let users know they've done something in conflict and what wins.
> I've been considering that over the weekend, and agree it is a good idea. I 
> am on the fence about the when however. For example, should we warn with 
> throw() or noexcept or noexcept(true)? In these cases, ignoring the attribute 
> is the same as enforcing it.
> 
> My concern is that noexcept (expression-evaluating-to-true) gets ignored with 
> a diagnostic, but has the same effect.
> 
> Did you have an opinion here?
I think the metric should be: if there are conflicting attributes where we need 
to ignore one of them, we should always diagnose when the attributes specify 
conflicting semantics, but we don't have to always diagnose when the attributes 
specify the same semantics. In this case, e.g.,
```
__declspec(nothrow) void foo1() noexcept; // Don't bother warning
__declspec(nothrow) void foo2() noexcept(true); // Don't bother warning
__declspec(nothrow) void foo3() noexcept(false); // Warn
__declspec(nothrow) void foo4() noexcept(foo1()); // Warning is debatable
__declspec(nothrow) void foo5() noexcept(foo3()); // Warn
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62435/new/

https://reviews.llvm.org/D62435



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

[PATCH] D62435: Add Attribute NoThrow as an Exception Specifier Type

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked an inline comment as done.
erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaType.cpp:6970
+if (Proto->hasExceptionSpec())
+  return true;
+

aaron.ballman wrote:
> I think we should diagnose that the `nothrow` is ignored in this case to let 
> users know they've done something in conflict and what wins.
I've been considering that over the weekend, and agree it is a good idea. I am 
on the fence about the when however. For example, should we warn with throw() 
or noexcept or noexcept(true)? In these cases, ignoring the attribute is the 
same as enforcing it.

My concern is that noexcept (expression-evaluating-to-true) gets ignored with a 
diagnostic, but has the same effect.

Did you have an opinion here?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62435/new/

https://reviews.llvm.org/D62435



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

[PATCH] D62435: Add Attribute NoThrow as an Exception Specifier Type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang-c/Index.h:227
+  /**
+   * The cursor has a declspec(nothrow) exception specification.
+   */

`__declspec(nothrow)`



Comment at: clang/include/clang/Basic/ExceptionSpecificationType.h:25
   EST_MSAny,///< Microsoft throw(...) extension
+  EST_NoThrow,  ///< Microsoft declspec(nothrow) extension
   EST_BasicNoexcept,///< noexcept

`__declspec(nothrow)`



Comment at: clang/lib/Sema/SemaType.cpp:6968
+
+// MSVC Ignores nothrow for exception specification if it is in conflict.
+if (Proto->hasExceptionSpec())

Ignores -> ignores



Comment at: clang/lib/Sema/SemaType.cpp:6970
+if (Proto->hasExceptionSpec())
+  return true;
+

I think we should diagnose that the `nothrow` is ignored in this case to let 
users know they've done something in conflict and what wins.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62435/new/

https://reviews.llvm.org/D62435



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

[PATCH] D62435: Add Attribute NoThrow as an Exception Specifier Type

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 201404.
erichkeane marked 2 inline comments as done.
erichkeane added a comment.

Reread @rsmith's comments and surrounding code and found what I believe is the 
correct answer to these comments :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62435/new/

https://reviews.llvm.org/D62435

Files:
  clang/include/clang-c/Index.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/ExceptionSpecificationType.h
  clang/include/clang/Sema/DeclSpec.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/Type.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/SemaCXX/nothrow-vs-exception-specs.cpp
  clang/tools/libclang/CXType.cpp

Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -742,6 +742,8 @@
 return CXCursor_ExceptionSpecificationKind_MSAny;
   case EST_BasicNoexcept:
 return CXCursor_ExceptionSpecificationKind_BasicNoexcept;
+  case EST_NoThrow:
+return CXCursor_ExceptionSpecificationKind_NoThrow;
   case EST_NoexceptFalse:
   case EST_NoexceptTrue:
   case EST_DependentNoexcept:
Index: clang/test/SemaCXX/nothrow-vs-exception-specs.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/nothrow-vs-exception-specs.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 %s -fcxx-exceptions -fsyntax-only -Wexceptions -verify -std=c++14
+// RUN: %clang_cc1 %s -fcxx-exceptions -fsyntax-only -Wexceptions -verify -std=c++17 -DCPP17
+
+__attribute__((nothrow)) void f1();
+static_assert(noexcept(f1()), "");
+void f1() noexcept;
+// expected-error@+2 {{exception specification in declaration does not match previous declaration}}
+// expected-note@-2 {{previous declaration is here}}
+void f1() noexcept(false);
+
+__attribute__((nothrow)) void f2();
+static_assert(noexcept(f2()), "");
+// expected-error@+2 {{exception specification in declaration does not match previous declaration}}
+// expected-note@-3 {{previous declaration is here}}
+void f2() noexcept(false);
+
+void f3() __attribute__((nothrow));
+static_assert(noexcept(f3()), "");
+void f3() noexcept;
+// expected-error@+2 {{exception specification in declaration does not match previous declaration}}
+// expected-note@-2 {{previous declaration is here}}
+void f3() noexcept(false);
+
+// Still noexcept due to throw()
+__attribute__((nothrow)) void f4() throw();
+static_assert(noexcept(f4()), "");
+
+// Still noexcept due to noexcept
+__attribute__((nothrow)) void f5() noexcept;
+static_assert(noexcept(f5()), "");
+
+// Still noexcept due to noexcept(true)
+__attribute__((nothrow)) void f6() noexcept(true);
+static_assert(noexcept(f6()), "");
+
+#ifndef CPP17
+// Doesn't override C++ implementation.
+__attribute__((nothrow)) void f7() throw(int);
+static_assert(!noexcept(f7()), "");
+#endif
+
+// Doesn't override C++ implementation.
+__attribute__((nothrow)) void f8() noexcept(false);
+static_assert(!noexcept(f8()), "");
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -130,6 +130,7 @@
   case ParsedAttr::AT_Regparm: \
   case ParsedAttr::AT_AnyX86NoCallerSavedRegisters:\
   case ParsedAttr::AT_AnyX86NoCfCheck: \
+  case ParsedAttr::AT_NoThrow: \
 CALLING_CONV_ATTRS_CASELIST
 
 // Microsoft-specific type qualifiers.
@@ -4516,7 +4517,7 @@
   // If the function declarator has a prototype (i.e. it is not () and
   // does not have a K identifier list), then the arguments are part
   // of the type, otherwise the argument list is ().
-  const DeclaratorChunk::FunctionTypeInfo  = DeclType.Fun;
+  DeclaratorChunk::FunctionTypeInfo  = DeclType.Fun;
   IsQualifiedFunction =
   FTI.hasMethodTypeQualifiers() || FTI.hasRefQualifier();
 
@@ -6945,6 +6946,38 @@
 return true;
   }
 
+  if (attr.getKind() == ParsedAttr::AT_NoThrow) {
+if (S.CheckAttrNoArgs(attr))
+  return true;
+
+// Delay if this is not a function type.
+if (!unwrapped.isFunctionType())
+  return false;
+
+// Otherwise we can process right away.
+auto *Proto = unwrapped.get()->getAs();
+
+// In the case where this is a FunctionNoProtoType instead of a
+// FunctionProtoType, let the existing NoThrowAttr implementation do its
+// thing.
+if (!Proto)
+  return false;
+
+attr.setUsedAsTypeAttr();
+
+// MSVC Ignores nothrow for exception specification if it is in conflict.
+if (Proto->hasExceptionSpec())
+  return true;
+
+type = 

[PATCH] D62435: Add Attribute NoThrow as an Exception Specifier Type

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 2 inline comments as done.
erichkeane added inline comments.



Comment at: clang/include/clang-c/Index.h:202-204
+   * The cursor has a declspec(nothrow) exception specification.
+   */
+  CXCursor_ExceptionSpecificationKind_NoThrow,

rsmith wrote:
> This would renumber the later enumerators, resulting in an ABI break for our 
> stable C ABI.
Is it alright to just add it to the end then? Or do I need to translate it into 
one of the others?



Comment at: clang/lib/Sema/SemaExprCXX.cpp:6067-6068
   if (EST2 == EST_NoexceptTrue) return ESI1;
+  if (EST1 == EST_NoThrow) return ESI2;
+  if (EST2 == EST_NoThrow) return ESI1;
 

rsmith wrote:
> I think this should be checked earlier, in the spirit of allowing "real" 
> syntax to be preferred to attributes. (Eg, given a declaration that's 
> `__declspec(nothrow)` and one that's `noexcept`, we should keep the 
> `noexcept` in the merged type.)
Ah, right, good point.

Looking above, I'm torn between above or below MSAny. Do you have a thought?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62435/new/

https://reviews.llvm.org/D62435



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

[PATCH] D62435: Add Attribute NoThrow as an Exception Specifier Type

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.

Seems like a nice approach to the problem, but we need to not break the 
libclang C ABI :)




Comment at: clang/include/clang-c/Index.h:202-204
+   * The cursor has a declspec(nothrow) exception specification.
+   */
+  CXCursor_ExceptionSpecificationKind_NoThrow,

This would renumber the later enumerators, resulting in an ABI break for our 
stable C ABI.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:6067-6068
   if (EST2 == EST_NoexceptTrue) return ESI1;
+  if (EST1 == EST_NoThrow) return ESI2;
+  if (EST2 == EST_NoThrow) return ESI1;
 

I think this should be checked earlier, in the spirit of allowing "real" syntax 
to be preferred to attributes. (Eg, given a declaration that's 
`__declspec(nothrow)` and one that's `noexcept`, we should keep the `noexcept` 
in the merged type.)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62435/new/

https://reviews.llvm.org/D62435



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

[PATCH] D62435: Add Attribute NoThrow as an Exception Specifier Type

[Erich Keane via Phabricator via cfe-commits]

erichkeane created this revision.
erichkeane added reviewers: rnk, lebedev.ri, aaron.ballman.
Herald added subscribers: cfe-commits, arphaman.
Herald added a project: clang.

In response to https://bugs.llvm.org/show_bug.cgi?id=33235, it became
clear that the current mechanism of hacking through checks for the
exception specification of a function gets confused really quickly when
there are alternate exception specifiers.

  

This patch introduces EST_NoThrow, which is the equivilent of
EST_noexcept when caused by EST_noThrow. The existing implementation is
left in place to cover functions with no FunctionProtoType.


Repository:
  rC Clang

https://reviews.llvm.org/D62435

Files:
  clang/include/clang-c/Index.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/ExceptionSpecificationType.h
  clang/include/clang/Sema/DeclSpec.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/Type.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/SemaCXX/nothrow-vs-exception-specs.cpp
  clang/tools/libclang/CXType.cpp

Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -742,6 +742,8 @@
 return CXCursor_ExceptionSpecificationKind_MSAny;
   case EST_BasicNoexcept:
 return CXCursor_ExceptionSpecificationKind_BasicNoexcept;
+  case EST_NoThrow:
+return CXCursor_ExceptionSpecificationKind_NoThrow;
   case EST_NoexceptFalse:
   case EST_NoexceptTrue:
   case EST_DependentNoexcept:
Index: clang/test/SemaCXX/nothrow-vs-exception-specs.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/nothrow-vs-exception-specs.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 %s -fcxx-exceptions -fsyntax-only -Wexceptions -verify -std=c++14
+// RUN: %clang_cc1 %s -fcxx-exceptions -fsyntax-only -Wexceptions -verify -std=c++17 -DCPP17
+
+__attribute__((nothrow)) void f1();
+static_assert(noexcept(f1()), "");
+void f1() noexcept;
+// expected-error@+2 {{exception specification in declaration does not match previous declaration}}
+// expected-note@-2 {{previous declaration is here}}
+void f1() noexcept(false);
+
+__attribute__((nothrow)) void f2();
+static_assert(noexcept(f2()), "");
+// expected-error@+2 {{exception specification in declaration does not match previous declaration}}
+// expected-note@-3 {{previous declaration is here}}
+void f2() noexcept(false);
+
+void f3() __attribute__((nothrow));
+static_assert(noexcept(f3()), "");
+void f3() noexcept;
+// expected-error@+2 {{exception specification in declaration does not match previous declaration}}
+// expected-note@-2 {{previous declaration is here}}
+void f3() noexcept(false);
+
+// Still noexcept due to throw()
+__attribute__((nothrow)) void f4() throw();
+static_assert(noexcept(f4()), "");
+
+// Still noexcept due to noexcept
+__attribute__((nothrow)) void f5() noexcept;
+static_assert(noexcept(f5()), "");
+
+// Still noexcept due to noexcept(true)
+__attribute__((nothrow)) void f6() noexcept(true);
+static_assert(noexcept(f6()), "");
+
+#ifndef CPP17
+// Doesn't override C++ implementation.
+__attribute__((nothrow)) void f7() throw(int);
+static_assert(!noexcept(f7()), "");
+#endif
+
+// Doesn't override C++ implementation.
+__attribute__((nothrow)) void f8() noexcept(false);
+static_assert(!noexcept(f8()), "");
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -130,6 +130,7 @@
   case ParsedAttr::AT_Regparm: \
   case ParsedAttr::AT_AnyX86NoCallerSavedRegisters:\
   case ParsedAttr::AT_AnyX86NoCfCheck: \
+  case ParsedAttr::AT_NoThrow: \
 CALLING_CONV_ATTRS_CASELIST
 
 // Microsoft-specific type qualifiers.
@@ -4516,7 +4517,7 @@
   // If the function declarator has a prototype (i.e. it is not () and
   // does not have a K identifier list), then the arguments are part
   // of the type, otherwise the argument list is ().
-  const DeclaratorChunk::FunctionTypeInfo  = DeclType.Fun;
+  DeclaratorChunk::FunctionTypeInfo  = DeclType.Fun;
   IsQualifiedFunction =
   FTI.hasMethodTypeQualifiers() || FTI.hasRefQualifier();
 
@@ -6945,6 +6946,38 @@
 return true;
   }
 
+  if (attr.getKind() == ParsedAttr::AT_NoThrow) {
+if (S.CheckAttrNoArgs(attr))
+  return true;
+
+// Delay if this is not a function type.
+if (!unwrapped.isFunctionType())
+  return false;
+
+// Otherwise we can process right away.
+auto *Proto = unwrapped.get()->getAs();
+
+// In the case where this is a