Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<bit>: Activate __cpp_lib_bitops for MSVC #313

Closed
StephanTLavavej opened this issue Nov 19, 2019 · 6 comments · Fixed by #795
Closed

<bit>: Activate __cpp_lib_bitops for MSVC #313

StephanTLavavej opened this issue Nov 19, 2019 · 6 comments · Fixed by #795
Assignees
Labels
enhancement Something can be improved fixed Something works now, yay!

Comments

@StephanTLavavej
Copy link
Member

#310 implemented #25 and #26:

  • WG21-P0553 <bit> Rotating And Counting Functions
  • WG21-P0556 <bit> ispow2(), ceil2(), floor2(), log2p1() bit_length()

This is currently active for Clang (and EDG, used for IntelliSense), but not MSVC, due to compiler support that has not yet been implemented:

STL/stl/inc/yvals_core.h

Lines 952 to 957 in 1980e1a

#if defined(__clang__) || defined(__EDG__)
#define __cpp_lib_bitops 201907L
#else // ^^^ Clang and EDG / MSVC vvv
// a future MSVC update will embed CPU feature detection into <bit> intrinsics
// TRANSITION, VSO-1020212
#endif // defined(__clang__) || defined(__EDG__)

When this compiler support is available, we should activate these features for MSVC.

@StephanTLavavej StephanTLavavej added enhancement Something can be improved blocked Something is preventing work on this labels Nov 19, 2019
@BurningEnlightenment
Copy link

Thanks for all the hard work. However

This is currently active for [...] IntelliSense), but not MSVC

From a user point of view it is a bit confusing that IntelliSense (in MSVC mode) happily auto completes/goes to the definition of the affected overload sets. While cl fails with the message that said overload sets are not part of the std namespace and points to completely unrelated headers. So I think it makes sense to expand defined(__EDG__) to (defined(__EDG__) && !defined(_MSC_VER)) or something similiar.
Fixing this isn't important, but OTOH it doesn't require much work. Thanks again 😸

@CaseyCarter
Copy link
Member

That is awkward; we should probably guard functionality that works in EDG but not MSVC with !defined(__INTELLISENSE__) to keep the IDE consistent with the compiler.

@StephanTLavavej
Copy link
Member Author

I think I'd prefer to just drop || defined(__EDG__) here until C1XX gains support, but I agree with the overall proposed change.

@AlexGuteniev
Copy link
Contributor

I'm confused about __cpp_lib_int_pow2 and __cpp_lib_bitops.

Currently, __cpp_lib_int_pow2 is defined and __cpp_lib_bitops is not.

What is the difference?

CaseyCarter added a commit to CaseyCarter/STL that referenced this issue Apr 8, 2020
* Don't define `__cpp_lib_bitops` when `__EDG__`, so IntelliSense is consistent with MSVC
* Don't define `__cpp_lib_int_pow2` when `__cpp_lib_bitops` isn't defined since the facilities that correspond with `__cpp_lib_int_pow2` are only provided when `__cpp_lib_bitops` is defined.

(See discussion starting at microsoft#313 (comment).)
@CaseyCarter
Copy link
Member

CaseyCarter commented Apr 8, 2020

When you need to know the history of a feature-test macro, or what features are covered by a feature-test macro, go straight to WG21 Standing Document 6 (SD-6). It is roughly 1000 times more useful than the working draft, which simply tells you what macros a conforming C++20 implementation defines and with what values.

__cpp_lib_int_pow2 corresponds to the facilities defined in WG21-P0556 "Integral power-of-2 operations" (ispow2, ceil2, floor2, and log2p1) which were later renamed by WG21-P1956 "On the names of low-level bit
manipulation functions" (has_single_bit - yes, really - bit_ceil, bit_floor, and bit_width).

__cpp_lib_bitops corresponds to WG21-P0553 "Bit operations" (rotl, rotr, countl_zero, countl_one, countr_zero, countr_one, and popcount).

The implementations of all of these functions rely on intrinsics that Clang supports but MSVC does not. We conditionally define __cpp_lib_bitops - which is proper, it should only be defined when the facilities with which it corresponds are available. However, we unconditionally define __cpp_lib_int_pow2 even though we do not always provide the corresponding functions which is most definitely a bug.

CaseyCarter added a commit that referenced this issue Apr 10, 2020
* Don't define `__cpp_lib_bitops` when `__EDG__`, so IntelliSense is consistent with MSVC
* Don't define `__cpp_lib_int_pow2` when `__cpp_lib_bitops` isn't defined since the facilities that correspond with `__cpp_lib_int_pow2` are only provided when `__cpp_lib_bitops` is defined.
* Update feature-test macro test accordingly

(See discussion starting at #313 (comment).)
@barcharcraz barcharcraz removed the blocked Something is preventing work on this label Jun 26, 2020
@barcharcraz barcharcraz self-assigned this Jun 26, 2020
@CaseyCarter
Copy link
Member

This was fixed by the merge of #795.

@CaseyCarter CaseyCarter added the fixed Something works now, yay! label Jul 2, 2020
@CaseyCarter CaseyCarter linked a pull request Jul 2, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved fixed Something works now, yay!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants