[libcxx-commits] [PATCH] D140632: [NFC][libc++] Removes concepts tests.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In D140632#4020164 <https://reviews.llvm.org/D140632#4020164>, @philnik wrote:

> In D140632#4018515 <https://reviews.llvm.org/D140632#4018515>, @Mordante wrote:
>
>> That might be true, but this part seems to work. So I really would like to land this.
>> We normally don't use feature-test macros in our test, instead we define a helper macro which we use in the tests.
>> Since it works it makes little sense to add a helper macro to do the right thing., s
>
> I'm not a huge fan of enabling the tests when we know that the implementation is still incomplete and buggy (which is the reason the FTM hasn't been bumped yet). Though I guess we do that already in other places, so I guess this looks good to me. We should reconsider that position though IMO, since it results in workarounds where nobody knows in the end why they existed in the first place.

I'm not against partly disabling tests when they don't work, but then we should use a libc++ specific test macro so we can enable/disable the feature based on our selection criteria. This is exactly how we have done it in the past, usually they were indeed based on a FTM but not always.

Thanks for the review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140632