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

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

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 (OldQTypeForComparison == NewQType ||
+        // In Microsoft compatibility mode, the intent is to only warn on
+        // mismatched exception specifiers.  By this point, that warning has
+        // already been issued, so we should treat mismatches only in exception
+        // specifier as equivalent.
+        (getLangOpts().MSVCCompat &&
+         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;
 

test/SemaCXX/ms-ignore-mismatched-except-specifier.cpp

+// RUN: %clang_cc1 %s -std=c++1z -fms-extensions -fms-compatibility
+
+template <typename T> struct FooPtr {
+  template <typename U> FooPtr(U *p) : m_pT(nullptr) {}
+  template <> FooPtr(T *pInterface) throw() : m_pT(pInterface) {}  // expected-warning {{exception specification in declaration does not match previous declaration}}
+  T *m_pT;
+};
+struct Bar{};
+template struct FooPtr<Bar>;