Properly ignore mismatched exception specifiers in MSVC Compat mode.

[rnk]

Summary

This fixes the error compiling _com_ptr_t described in https://bugs.llvm.org/show_bug.cgi?id=42842

In MSVCCompat mode, clang already attempts to downgrade these mismatches to a warning, but because C++17 considers the specifiers part of the type, the check in MergeFunctionDecl needs to ignore these specifiers for purpose of comparison.

[rnk]

lib/Sema/SemaDecl.cpp

3565–3572 ↗

(On Diff #220238)

I wonder if we should simplify this to just call hasSameFunctionTypeIgnoringExceptionSpec in all modes. Essentially, just trust that CheckExceptionSpecification did the right thing, in case it returns false for other reasons in the future. It might actually be better for error recovery, for example, to continue adding this overload when exception specifications don't match. I'll try that and see if it passes tests.

[alexfusco]

Updated to unconditionally ignore the exception spec here, since it has already been compared.

[rnk]

Taking this to move the test around and try the other version...

[rnk]

• move test

[alexfusco]

clang/test/SemaCXX/ms-exception-spec.cpp

2 ↗

(On Diff #220376)

Dynamic exception specifiers are also an error in C++17, which causes issues on line 4 and 8 in this invocation

14 ↗

(On Diff #220376)

There's also a (pre-existing and incorrect) note here that this is also the previous declaration and a note on line 18 that it's in instantiation of that template.

[zahen]

Is there any interest in supporting the cl.exe flag /permissive-? I considered a hard error on mismatched exception specifier in clang-cl a feature, not a bug. If msvc compat mode respected that flag this could continue to be an error with that flag set (and upgraded strictness in other cases).

[thakis]

In D67590#1672691, @zahen wrote:

Is there any interest in supporting the cl.exe flag /permissive-? I considered a hard error on mismatched exception specifier in clang-cl a feature, not a bug. If msvc compat mode respected that flag this could continue to be an error with that flag set (and upgraded strictness in other cases).

Does /permissive- in cl.exe turn this specific thing into an error? If not, we don't want to add this under that flag.

You can always use -Werror=microsoft-exception-spec to turn this into an error.

This behavior is necessary to parse system headers, and the general strategy for system headers is "accept enough to be able to parse system headers, but emit a warning". https://docs.google.com/presentation/d/1oxNHaVjA9Gn_rTzX6HIpJHP7nXRua_0URXxxJ3oYRq0/edit#slide=id.p has some more words on this (slides 26/27).

[zahen]

I just checked a trivial mismatch example in clang 8.0 (https://godbolt.org/z/wiSAp6) and I missed how the compatibility mode operates. I withdraw my suggestion since the patch keeps consistency with the existing behavior. Thanks for the -Werror=microsoft-exception-spec suggestion that's exactly what our project wants.

[rnk]

• simplify merging check

cfe/trunk/lib/Sema/SemaDecl.cpp

if (haveIncompatibleLanguageLinkages(Old, New)) {
         Diag(OldLocation, PrevDiag);
       } else {
         Diag(New->getLocation(), diag::err_different_language_linkage) << New;
         Diag(OldLocation, PrevDiag);
         return true;
       }
     }
 
-    if (OldQTypeForComparison == NewQType)
+    // If the function types are compatible, merge the declarations. Ignore the
+    // exception specifier because it was already checked above in
+    // CheckEquivalentExceptionSpec, and we don't want follow-on diagnostics
+    // about incompatible types under -fms-compatibility.
+    if (Context.hasSameFunctionTypeIgnoringExceptionSpec(OldQTypeForComparison,
+                                                         NewQType))
       return MergeCompatibleFunctionDecls(New, Old, S, MergeTypeWithOld);
 
     // If the types are imprecise (due to dependent constructs in friends or
     // local extern declarations), it's OK if they differ. We'll check again
     // during instantiation.
     if (!canFullyTypeCheckRedeclaration(New, Old, NewQType, OldQType))
       return false;
 

cfe/trunk/test/SemaCXX/ms-exception-spec.cpp

-// RUN: %clang_cc1 %s -fsyntax-only -verify -fms-compatibility -fexceptions -fcxx-exceptions
+// RUN: %clang_cc1 -std=c++11 %s -fsyntax-only -verify -fms-compatibility -fexceptions -fcxx-exceptions
+// RUN: %clang_cc1 -std=c++17 %s -fsyntax-only -verify -fms-compatibility -fexceptions -fcxx-exceptions
 
+// FIXME: Should -fms-compatibility soften these errors into warnings to match
+// MSVC? In practice, MSVC never implemented dynamic exception specifiers, so
+// there isn't much Windows code in the wild that uses them.
+#if __cplusplus >= 201703L
+// expected-error@+3 {{ISO C++17 does not allow dynamic exception specifications}}
+// expected-note@+2 {{use 'noexcept(false)' instead}}
+#endif
 void f() throw(...) { }
 
 namespace PR28080 {
 struct S;           // expected-note {{forward declaration}}
+#if __cplusplus >= 201703L
+// expected-error@+3 {{ISO C++17 does not allow dynamic exception specifications}}
+// expected-note@+2 {{use 'noexcept(false)' instead}}
+#endif
 void fn() throw(S); // expected-warning {{incomplete type}} expected-note{{previous declaration}}
 void fn() throw();  // expected-warning {{does not match previous declaration}}
 }
+
+template <typename T> struct FooPtr {
+  template <typename U> FooPtr(U *p) : m_pT(nullptr) {}
+
+  template <>
+      // FIXME: It would be better if this note pointed at the primary template
+      // above.
+      // expected-note@+1 {{previous declaration is here}}
+  FooPtr(T *pInterface) throw() // expected-warning {{exception specification in declaration does not match previous declaration}}
+      : m_pT(pInterface) {}
+
+  T *m_pT;
+};
+struct Bar {};
+template struct FooPtr<Bar>; // expected-note {{requested here}}