This macro makes it possible to conditionally enable code when 128-bit
integral support is available. This improves the readablity of the
tests. (This patch is also intended to be used in new format tests.)
Details
- Reviewers
ldionne - Group Reviewers
Restricted Project
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I will admit I'm not convinced that we're saving much here, in particular I'm a bit concerned with the fact that these IF(...) macros are less usual (if more terse) than the basic #if defined(...), and so one might have to look it up.
All in all I'm not strongly against, and I especially don't want to block you if it makes a big difference in upcoming std::format tests, but if the gain in std::format is not compelling, I'd rather not do this. If it's a compelling gain for std::format, I can definitely live with this.
I agree with what Louis said: the cost/benefit seems not favorable at the moment. (I hadn't looked at this yet because I assumed it was adding the-equivalent-of-TEST_HAS_NO_INT128 — I forgot we already had one of those!)
If you do land this, could you name it TEST_IF_HAS_INT128?
For format I use it in a few places. The difference there is it's at the end of the list which makes it more awkward to use #ifdef due to the "hanging" >. For example
std::tuple< unsigned long long, long long, unsigned long, long, unsigned int, int, unsigned short, short, unsigned char, signed char #ifndef TEST_HAS_NO_INT128 , __int128_t, __uint128_t #endif > types;
The reason to put them at the end is that the test does something like auto foo = std::get<1>(types).
Regarding the name, I based the name on _LIBCPP_IF_WIDE_CHARACTERS which also lacks _HAS. I prefer to keep things consistent. @ldionne @Quuxplusone do you prefer 1 or 2?
- _LIBCPP_IF_WIDE_CHARACTERS and TEST_IF_INT128
- _LIBCPP_IF_HAS_WIDE_CHARACTERS and TEST_IF_HAS_INT128
Based on Arthur's suggestion I'm now more tending to option 2.
Sounds like std::get<1>(types) is the root of the problem. Since you know get<1>(types) is always long long, why not replace the magic number 1 with the "magic number" long long? Then the test would be more robust not only against people adding uint128_t at the beginning of the list, but also against people sorting the list, or adding char16_t, or whatever other permutations they might do under maintenance. (Btw, I can imagine a reason it wouldn't be quite that simple: maybe the type list contains both long long and int64_t, so long long would be ambiguous while 1 is unambiguous. But still.)
Also, how come these format-uses aren't part of this PR? Are they not landed yet? Could we defer this PR until after the format-uses are landed, and/or take this (currently too-abstract) discussion over to that PR?
- _LIBCPP_IF_WIDE_CHARACTERS and TEST_IF_INT128
- _LIBCPP_IF_HAS_WIDE_CHARACTERS and TEST_IF_HAS_INT128
Based on Arthur's suggestion I'm now more tending to option 2.
Weakly option 2. However, I remain unconvinced that the macro is needed at all.
I'll leave this open for the other use case.
@ldionne which do you prefer for naming 1 or 2? When also 2 I'll make a separate patch to s/_LIBCPP_IF_WIDE_CHARACTERS/_LIBCPP_IF_HAS_WIDE_CHARACTERS/
As discussed: When this patch doesn't carry its weight for D121514 we won't land this.
clang-format suggested style edits found: