[libcxx-commits] [PATCH] D76256: [libc++] Require the use of clang-verify in .fail.cpp tests that don't fail without it

Mon Mar 23 08:09:22 PDT 2020

EricWF accepted this revision.
EricWF added a comment.
This revision is now accepted and ready to land.

In D76256#1930417 <https://reviews.llvm.org/D76256#1930417>, @ldionne wrote:

> In D76256#1930359 <https://reviews.llvm.org/D76256#1930359>, @EricWF wrote:
>
> > 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76256