Page MenuHomePhabricator

Fix regression in special member definitions under SuppressAllDiagnostics
AbandonedPublic

Authored by brad.king on Sep 1 2017, 8:34 AM.

Details

Reviewers
rsmith
Summary

In commit r303930 (Switch from using a DiagnosticTrap and a note...,
2017-05-25) use of DiagnosticErrorTrap was removed because it was no
longer needed to detect whether to add a diagnostic note. However, the
trap was also necessary to correctly detect and mark invalid special
members under Sema.getDiagnostics().setSuppressAllDiagnostics(true).
Restore use of the trap to fix semantic checking when diagnostics are
suppressed.

This is useful for external tools that need to detect the set of valid
special members without issuing diagnostics on the invalid ones.
Add a unit test that does this and verifies the results.

Diff Detail

Event Timeline

brad.king created this revision.Sep 1 2017, 8:34 AM
  1. Please upload path with full context (-U9999)
  2. Tests?
brad.king updated this revision to Diff 113544.Sep 1 2017, 8:53 AM

Updated diff with full context.

Tests?

make check didn't regress from this.

This behavior difference is not observable from within Clang itself, only in external applications that use SuppressAllDiagnostics while forcing definition of implicit members. Adding a test will require reproducing a chunk of the application code in the test suite to create the appropriate CompilerInstance. I'd be happy to do that to keep this working in the future, but where would be the appropriate place?

BTW, I'd like to see this integrated in the release_50 branch because this regression breaks my application that works with 3.6, 3.7, 3.8, 3.9, and 4.0.

Tests?

make check didn't regress from this.

Which is the problem.

Say this gets committed, and then someone else for some reason reverts part of the patch.
Normally, tests should catch that. But as you have stated yourself, no tests failed because of this...

This behavior difference is not observable from within Clang itself, only in external applications that use SuppressAllDiagnostics while forcing definition of implicit members. Adding a test will require reproducing a chunk of the application code in the test suite to create the appropriate CompilerInstance. I'd be happy to do that to keep this working in the future, but where would be the appropriate place?

BTW, I'd like to see this integrated in the release_50 branch because this regression breaks my application that works with 3.6, 3.7, 3.8, 3.9, and 4.0.

brad.king updated this revision to Diff 113561.Sep 1 2017, 11:32 AM
brad.king edited the summary of this revision. (Show Details)

Updated diff to add a test case.

rsmith requested changes to this revision.Sep 1 2017, 4:41 PM

It's not reasonable for external users of Clang's AST to infer deep meaning from the "invalid" flag. It's simply a mechanism by which we (a) limit the amount of follow-on diagnostics, and (b) somewhat weaken our AST invariants in the presence of invalid code; it is an implementation detail and subject to change at any time. (If a node is not marked invalid, that does not imply the corresponding source code is valid.)

If you would like a system to hook into the start and end of creating a function (so you can install your own error trap from an external user of Sema), that would seem like something we could reasonably support.

This revision now requires changes to proceed.Sep 1 2017, 4:41 PM
brad.king abandoned this revision.Sep 6 2017, 4:26 AM

@rsmith thanks.

If a node is not marked invalid, that does not imply the corresponding source code is valid.

Okay, that clears up my understanding.

hook into the start and end of creating a function (so you can install your own error trap from an external user of Sema)

I only need a way to detect special members in C++98 that won't compile (since they won't be isDeleted), not trap bad code in general. I found that I can install my own error trap around my code that forces the definition of each special member.

Therefore we can abandon this change. Thanks!