Page MenuHomePhabricator

adds [concepts.arithmetic]
ClosedPublic

Authored by cjdb on Sep 22 2020, 8:23 PM.

Details

Summary

Adds:

  • std::integral
  • std::signed_integral
  • std::unsigned_integral
  • std::floating_point

Diff Detail

Event Timeline

cjdb created this revision.Sep 22 2020, 8:23 PM
cjdb requested review of this revision.Sep 22 2020, 8:23 PM
cjdb added inline comments.
libcxx/test/std/concepts/lang/arithmetic.pass.cpp
30

This formatting feels off but it's what git clang-format-12 HEAD~1 gave me. Can someone please confirm this is the correct style?

Some small nits

libcxx/test/std/concepts/lang/arithmetic.pass.cpp
30

I believe that there is no libc++ code style yet, @ldionne what are the chances of defining one?

31

Would it simplify the testing when we had a function like this

cpp
template <typename T, bool Expected>
constexpr void CheckQualifiers() {
     static_assert(std::integral<T> == Expected);
     static_assert(std::integral<const T> == Expected);
     static_assert(std::integral<volatile T> == Expected);
     static_assert(std::integral<const volatile T> == Expected);

     static_assert(!std::integral<T&>);
     static_assert(!std::integral<const T&>);
     static_assert(!std::integral<volatile T&>);
     static_assert(!std::integral<const volatile T&>);

     static_assert(!std::integral<T*>);
     static_assert(!std::integral<const T*>);
     static_assert(!std::integral<volatile T*>);
     static_assert(!std::integral<const volatile T*>);
}
45

I believe this counts as extended integer types?

CaseyCarter added inline comments.Sep 28 2020, 2:29 PM
libcxx/include/concepts
164

Pre-existing: did GCC start supporting __is_convertible_to while I wasn't looking, or is this clang-only?

libcxx/test/std/concepts/lang/arithmetic.pass.cpp
30

I'd call these "standard signed and unsigned integer types".

31

Having written a few such functions, I suggest a slight modification to:

template <typename T>
constexpr bool CheckQualifiers() {
    constexpr bool result = std::integral<T>;
    static_assert(std::integral<const T> == result);
    static_assert(std::integral<volatile T> == result);
    static_assert(std::integral<const volatile T> == result);

    static_assert(!std::integral<T&>);
    static_assert(!std::integral<const T&>);
    static_assert(!std::integral<volatile T&>);
    static_assert(!std::integral<const volatile T&>);

    static_assert(!std::integral<T*>);
    static_assert(!std::integral<const T*>);
    static_assert(!std::integral<volatile T*>);
    static_assert(!std::integral<const volatile T*>);

    return result;
}

so you can more easily static_assert(CheckQualifiers<int>()); static_assert(!CheckQualifiers<void(*)(int) const>);.

45

"Extended integer types" are implementation-defined types with the same properties as integral types; __int128 is almost an extended integer type, for example. Formally, these are "integer types that are neither signed nor unsigned integer types" - which is far from a fantastic name. The colloquial "character or bool types" is probably clearer to folks who don't list Standardese as their first language.

76–78

None of these Sneaky members are used - they could be elided. Of course it's then odd to have a dozen-ish empty structs with different names; you could as well use EmptyStruct for all of these. (A type T doesn't need to have any members of type U for us to form the pointer-to-member type U T::*.)

cjdb updated this revision to Diff 318085.Jan 20 2021, 6:36 PM
cjdb marked 7 inline comments as done.
cjdb marked an inline comment as done.Jan 20 2021, 6:38 PM

@CaseyCarter @miscco @ldionne @EricWF I think this has addressed all the comments and should be good to merge.
Input from @CaseyCarter on the new CheckSubsumption appreciated, since that check always trips me up.

libcxx/include/concepts
164

I've done something weird here with Git because D74292 is still open.

libcxx/test/std/concepts/lang/arithmetic.pass.cpp
76–78

Thanks, that simplifies things a lot!

cjdb set the repository for this revision to rG LLVM Github Monorepo.Feb 4 2021, 12:06 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptFeb 4 2021, 12:06 PM
cjdb updated this revision to Diff 321538.Feb 4 2021, 12:07 PM
cjdb marked an inline comment as done.

Activates CI

Activates CI

Are you using arc diff to upload patches? The Phab -> BuildKite bridge seems to have trouble applying your patches, not sure why.

cjdb updated this revision to Diff 321971.Sat, Feb 6, 7:22 PM

Rebased for CI

cjdb added a comment.Sat, Feb 6, 7:27 PM

Activates CI

Are you using arc diff to upload patches? The Phab -> BuildKite bridge seems to have trouble applying your patches, not sure why.

No, I've been using the Phab web interface. Do you recommend I try the CLI tool instead?

Are you using arc diff to upload patches? The Phab -> BuildKite bridge seems to have trouble applying your patches, not sure why.

No, I've been using the Phab web interface. Do you recommend I try the CLI tool instead?

Without the CLI tool the CI will not automatically rebuild the new diff. This is an issue with Phabricator.

libcxx/include/concepts
208

Interesting to it written like this, it compares integral<_Tp> twice. (Just a remark, since this matches the Standard.)

libcxx/test/std/concepts/lang/arithmetic.pass.cpp
73

Can you also test __int128_t and __uint128_t where applicable? This can be guarded with #ifndef _LIBCPP_HAS_NO_INT128.

cjdb updated this revision to Diff 322008.Sun, Feb 7, 5:00 PM
cjdb edited the summary of this revision. (Show Details)

addresses CI issues and Mordante's request

cjdb marked 2 inline comments as done.Sun, Feb 7, 5:00 PM
cjdb added inline comments.
libcxx/include/concepts
208

This is for subsumption, see my tests below :-)

ldionne requested changes to this revision.Tue, Feb 9, 3:21 PM

Mostly nitpicks, except the part about intrinsics which I would really like to de-duplicate.

libcxx/include/concepts
201

Same comment about not duplicating the implementation to use intrinsics, since they are already used by the type traits.

libcxx/test/std/concepts/lang/arithmetic.pass.cpp
31

Really clever.

166

No need for these comments for small #if pairs.

265

Is that the result of clang-format? This seems weirdly aligned, no?

This revision now requires changes to proceed.Tue, Feb 9, 3:21 PM
cjdb updated this revision to Diff 322556.Tue, Feb 9, 5:49 PM
cjdb marked 4 inline comments as done.

removes conditional concept definitions as disucssed with Louis offline

libcxx/test/std/concepts/lang/arithmetic.pass.cpp
265

Yeah, I believe there's some new settings in clang-format-13 to handle concepts, requires-expressions, and requires-clauses properly. For the time being, I've just added some much-hated comments :-)

ldionne accepted this revision.Wed, Feb 10, 7:09 AM
This revision is now accepted and ready to land.Wed, Feb 10, 7:09 AM
This revision was automatically updated to reflect the committed changes.