[PATCH] D55039: [sema] Warn of mangling change if function parameters are noexcept.

[Matt Davis via Phabricator via cfe-commits]

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


================
Comment at: lib/Sema/SemaDecl.cpp:10266
       auto *FPT = NewFD->getType()->castAs<FunctionProtoType>();
-      bool AnyNoexcept = HasNoexcept(FPT->getReturnType());
-      for (QualType T : FPT->param_types())
-        AnyNoexcept |= HasNoexcept(T);
-      if (AnyNoexcept)
+      if (hasNoexcept(FPT->getReturnType()) ||
+          llvm::any_of(FPT->param_types(),
----------------
mattd wrote:
> Rakete1111 wrote:
> > What you're doing here is a subset of what `hasNoexcept` is doing for function types. Do you think it would make sense to use `!FPT->isNothrow() && hasNoexcept(NewFW->getType())` or something like that?
> Thanks for the comments @Rakete1111.
> 
> I do see the similarity and was trying to implement a solution to take advantage of that.  However, I was running into trouble on assert/debug builds.  `llvm_unreachable`, called in `FunctionProtoType::canThrow`, will trigger on FunctionProtoType instances that have not yet had their exception-spec type evaluated.  `canThrow` is called on behalf of `isNothrow`.  Some of the test cases will fail in assert/debug builds if we place a call to to `FPT->isNothrow` here. 
> 
> The original code avoided this assertion by not calling `isNothrow` on FPT here, but instead just evaluating the return and parameter types of FPT.  I made this patch similar to what was already in place, but with the more  thorough check in `hasNoexcept.`  I'd be pleased to remove this duplication/similarity if I can avoid this assertion.
I just got back into looking at this issue.  Given that I was unable to implement `!FPT->isNoThrow() && hasNoexcept(NewFW->getType())` without triggering an assertion (discussed in my reply immediately above), is there anything we should be doing differently here?  The change, as it sits now (with the duplication), seems to work fine.


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

https://reviews.llvm.org/D55039