Page MenuHomePhabricator

[libc++] Require the use of clang-verify in .fail.cpp tests that don't fail without it
ClosedPublic

Authored by ldionne on Mar 16 2020, 2:59 PM.

Details

Summary

Some tests do not fail at all when -verify is not supported, unless some
arbitrary warning flag is added to make them fail. We currently used
-Werror=unused-result to make them fail, but doing so makes the test
suite a lot more inscrutable. It seems better to just disable those
tests when -verify is not supported.

Diff Detail

Event Timeline

ldionne created this revision.Mar 16 2020, 2:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2020, 2:59 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
EricWF added a subscriber: EricWF.Mar 18 2020, 8:03 PM

These tests still require that "arbitrary" warning flag be enabled.

I'm also a bit afraid that the verify-support tag will get accidentally disabled and these tests along with it.
Also, do we care about GCC?

These tests still require that "arbitrary" warning flag be enabled.

I don't understand? The only reason these tests were using that flag was to cause a compilation error (instead of a warning) on GCC. If we properly require that we're able to check the diagnostics, which is the goal, we can stop trying to make them fail arbitrarily on GCC.

Basically, it's kind of as if we had said "for these tests (and these tests only), enable -Werror=unused-typedef on GCC cause we know that's going to make them fail. Instead, my point is that we can just disable them cause they're not doing much good.

I'm also a bit afraid that the verify-support tag will get accidentally disabled and these tests along with it.

I think we need to trust our config.py here. I agree it's complicated and I'm working on fixing it, but this change is actually one step exactly in that direction.

Also, do we care about GCC?

These tests already don't do much on GCC, they just confirm that -Werror-unused-result works properly.

EricWF accepted this revision.Mar 23 2020, 8:04 AM

These tests still require that "arbitrary" warning flag be enabled.

I don't understand? The only reason these tests were using that flag was to cause a compilation error (instead of a warning) on GCC. If we properly require that we're able to check the diagnostics, which is the goal, we can stop trying to make them fail arbitrarily on GCC.

Previously these tests required -Werror=-Wunused-result. Now they require -Wunused-result. The requirement for an arbitrary flag remains, only the spelling changed.
That said, I'm fine with this.

Basically, it's kind of as if we had said "for these tests (and these tests only), enable -Werror=unused-typedef on GCC cause we know that's going to make them fail. Instead, my point is that we can just disable them cause they're not doing much good.

I'm also a bit afraid that the verify-support tag will get accidentally disabled and these tests along with it.

I think we need to trust our config.py here. I agree it's complicated and I'm working on fixing it, but this change is actually one step exactly in that direction.

Every Clang we test against should support -verify. How about we simply say // UNSUPPORTED: gcc?
That would assuage my concern.

Also, do we care about GCC?

These tests already don't do much on GCC, they just confirm that -Werror-unused-result works properly.

LGTM if we agree on the // UNSUPPORTED: gcc change.

This revision is now accepted and ready to land.Mar 23 2020, 8:04 AM

These tests already don't do much on GCC, they just confirm that -Werror-unused-result works properly.

LGTM if we agree on the // UNSUPPORTED: gcc change.

So, more generally, would you be OK with dropping the notion of // REQUIRES: verify-support and instead translate that into // UNSUPPORTED: gcc? I believe the best would be to simplify the logic for how we currently determine the verify-support feature by just checking whether we're using GCC, but leave // REQUIRES: verify-support as that provides more information semantically than // UNSUPPORTED: gcc. WDYT? If we're on the same page, I can do that as a follow up change.

These tests already don't do much on GCC, they just confirm that -Werror-unused-result works properly.

LGTM if we agree on the // UNSUPPORTED: gcc change.

So, more generally, would you be OK with dropping the notion of // REQUIRES: verify-support and instead translate that into // UNSUPPORTED: gcc? I believe the best would be to simplify the logic for how we currently determine the verify-support feature by just checking whether we're using GCC, but leave // REQUIRES: verify-support as that provides more information semantically than // UNSUPPORTED: gcc. WDYT? If we're on the same page, I can do that as a follow up change.

I'll check this in as-is and we can talk offline about exactly what you mean by // UNSUPPORTED: gcc. Whatever change we make, we should make it for all tests that have // REQUIRES: verify-support.

This revision was automatically updated to reflect the committed changes.