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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete1111 added a comment.

Apart from the comments I think your patch LGTM, but I'll let someone else hav the final say.



================
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:
> 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.
I think it's fine.


================
Comment at: lib/Sema/SemaDecl.cpp:10268
+          llvm::any_of(FPT->param_types(),
+                       [](QualType Q) { return hasNoexcept(Q); })) {
         Diag(NewFD->getLocation(),
----------------
Rakete1111 wrote:
> Same, that lambda is unnecessary.
You marked this comment as done but the lambda is still there :).


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

https://reviews.llvm.org/D55039