[PATCH] D39013: [CFG] Relax Wexceptions warning on rethrow

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D39013#900046, @erichkeane wrote:

> I've convinced myself that a re-throw with a catch around it should not be diagnosed, otherwise there is no way to suppress the warning in this case.  It ends up being a false-positive in many cases.


Wouldn't a catch-all handler suppress the warning in that case? e.g.,

  void f() noexcept {
    try {
      throw; // rethrows
    } catch (int) {
      // We hope this catches it, and if it always does, then this is a fp warning.
    } catch (...) {
      // However, without cross-TU analysis, we can't know that detail and it seems plausible that
      // the active exception object is something else, so the catch-all is not unreasonable and adds
      // extra safety. The diagnostic (even if a fp) requires the developer to think about this case.
      assert(false);
    }
  }



================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:299
   if (!ThrowType)
     return false;
   if (ThrowType->isReferenceType())
----------------
erichkeane wrote:
> The other potential fix here is to simply change this line here to "return true;".  This would result the warning being suppressed on 'rethrow' if there is any catch statements. 
> 
> I wonder what the opinion of the reviewers is on this one.
I'm a little bit concerned about it, truth be told. The current behavior has false positives, and this patch trades those for false negatives. The question boils down to which is the more likely scenario. Rethrow from within a try block that catches the exception being rethrown seems like a rather unlikely scenario to me. Do you have examples of this being a false positive with real-world code?

I do agree that the catch-all handler should silence the diagnostic, however.


https://reviews.llvm.org/D39013