[xutility] Modernize _Equal_memcmp_is_safe to use variable templates #831

miscco · 2020-05-13T19:07:22Z

This moves the helper structs _Equal_memcmp_is_safe and _Equal_memcmp_is_safe_helper to use variable templates rather than structs.

This is a precursor to #819

miscco · 2020-05-14T11:56:50Z

Just one more round is great for civilization bot not good for PRs. This should now be ready for initial review.

mnatsuhara · 2020-05-14T17:51:11Z

stl/inc/xutility

    return _Equal_unchecked(_UFirst1, _ULast1, _UFirst2, _Pass_fn(_Pred));
 }
-#endif // _HAS_IF_CONSTEXPR
+#endif // !_HAS_IF_CONSTEXPR


Is this what we typically do? If you look at the example just below (line 4793-4871 before changes), the #endif is commented with the original condition (not negated). Doing a quick search throughout the code base also shows that the original comment follows our convention.

As far as I know there was a discussion early after open sourcing and I have the memory that the majority was in favor of

#if foo #else // ^^^ foo / !foo vvv #endif // !foo

The reason was that for longer clauses such as this just reading #endif // foo would lead you to believe that foo is active or require you to check the start of the block.

That said sleep was rather short this year so I doubt any memory I have

This is #351 - existing code has accumulated inconsistent patterns. I'm not sure how we ended up with the "hybrid arrow" examples that @mnatsuhara pointed out - I think they evolved from our classic pattern of

#if _HAS_CATS #else // _HAS_CATS #endif // _HAS_CATS #ifdef __cpp_lib_kittens #else // __cpp_lib_kittens #endif // __cpp_lib_kittens

by replacing just the middle portion with arrows.

My personal opinion: Aside from the question of when comments can be omitted, I'd prefer for us to standardize on consistent arrow comments:

#if _HAS_CATS #else // ^^^ _HAS_CATS ^^^ / vvv !_HAS_CATS vvv #endif // ^^^ !_HAS_CATS ^^^ #ifdef __cpp_lib_kittens #else // ^^^ defined(__cpp_lib_kittens) ^^^ / vvv !defined(__cpp_lib_kittens) vvv #endif // ^^^ !defined(__cpp_lib_kittens) ^^^

(I'm not very concerned with whether ^^^ / vvv or just / appears in the middle; I like the former slightly more.) I prefer this because it's the clearest in dense regions of preprocessor logic, and it's also unambiguous about whether the final comment should say _HAS_CATS or !_HAS_CATS (because it points to a specific section of code).

I could see an argument that "of course an #endif closes a section of code, so we just need to consistently identify what we closed", which would argue for !_HAS_CATS here.

Somewhat related: I believe we've accumulated some inconsistency about how to refer to #ifdef (and #if defined(MEOW) etc.) conditions in comments - I have seen some comments switch from the defined(MEOW) / !defined(MEOW) domain to the MEOW / !MEOW domain which I find confusing. This is because of the danger of actually confusing the definedness versus valueness domains in the test (and the compiler warning about #if NOT_DEFINED being treated as #if 0).

There are "only" 444 occurrences of #else in stl/inc, but 2049 #endifs - tedious to audit, but still easier than bracing all control flow, which we finally did and it was awesome. I should probably just submit a PR to prevent this from burning up valuable time in the future. 🔥

I have to agree that we should be consistent. As I had to rebase anyway I removed that change so that it can be tackled with all the other comments in a consistent fashion

Maybe this would be a "good second issue"

stl/inc/algorithm

stl/inc/xutility

tests/std/tests/VSO_0180469_ptr_cat/test.cpp

miscco · 2020-05-20T06:21:38Z

I am still kind of stuck here. My problem with the top level const is that I cannot really constrain the _Equal_memcmp_is_safe because if I use enable_if_t as an additional template argument, it complains about the invalid specialization. If I use enable_if_t on one of the arguments of the specialization it errors out because one of the template argument would be unused in the SNFINAE case.

Given that this is essentially soon to be reworked code I am actually considering just hand rolling the 4 combinations of _Elem* and _Elem* const and be done with.

StephanTLavavej · 2020-05-20T10:09:18Z

Ah, I see - it's a partial specialization of a variable template, so you can't really interfere with the partial-specialization-ness.

What about the following? (I can prepare a fully worked example later if you want.) Give the primary template a default template argument of the form bool = conjunction_v<is_pointer<ITER1>, is_pointer<ITER2>> and make that the "true" version, which then uses remove_pointer_t to remove any top-level const, followed by the pointer, and then deals with the (possibly const) element type. Then, provide a partial specialization for <ITER1, ITER2, false> that just answers false. (This reverses the current pattern, because of the stylistic preference of having the primary template be used for a true bool.)

miscco · 2020-05-20T11:27:20Z

@StephanTLavavej thanks a lot that is exactly what I needed 😲

miscco

I again managed to screw the push up, the review comments should now all be satisfied and this ready for final review

stl/inc/xutility

tests/std/tests/VSO_0180469_ptr_cat/test.cpp

stl/inc/xutility

miscco · 2020-05-23T17:15:00Z

@CaseyCarter this should now close #819

CaseyCarter · 2020-05-23T18:10:44Z

@CaseyCarter this should now close #819

Thanks for the reminder; I've attached #819 and tagged it as "work in progress."

miscco · 2020-05-28T06:30:35Z

@StephanTLavavej Thanks for improving the comments

StephanTLavavej · 2020-05-29T00:51:07Z

I am working on overhauling the metaprogramming - I believe I verified the correctness of your additions yesterday, but the original design was messy to extend and it was exceeding my ability to easily reason about. I think I have something simpler that preserves your additions and enhancements, and avoids replicating too much code for concepts and non-concepts. Will add comments, test, and push soon.

miscco · 2020-05-29T05:16:57Z

Given that you had trouble we can estimate hoe long I struggled to get something working.

Thanks a lot for improving it further

miscco

That is indeed so much better. Thanks a lot

stl/inc/xutility

StephanTLavavej

I'm fixing the test coverage.

tests/std/tests/VSO_0180469_ptr_cat/test.cpp

Include `<cstddef>` for `byte`. Test it when it's available. Style: Use `is_unsigned_v`. Add more test cases for mixed `int` and `long`. Remove a duplicated line (the cv pattern is clear).

tests/std/tests/VSO_0180469_ptr_cat/test.cpp

StephanTLavavej · 2020-05-29T10:16:35Z

I'm porting this to MSVC and will need to update that internal PR if any more changes are pushed here.

stl/inc/xutility

StephanTLavavej · 2020-05-30T00:23:39Z

Thanks for enhancing this optimization! Now, users of C++20 ranges will get the same codegen as for classic algorithms. 😸

miscco · 2020-05-30T05:29:22Z

I would also like to thank me for pushing you to write awesome code

cpplearner · 2020-11-22T11:23:04Z

This seems to cause DevCom-1263877 (std::equal does not work with custom contiguous iterators).

#1433 contains a fix, but a smaller PR might be preferable.

AdamBucior · 2020-11-22T11:53:11Z

I don't think smaller PR would be preferable, #1433 is not huge.

miscco requested a review from a team as a code owner May 13, 2020 19:07

miscco force-pushed the equal_memcmp branch from 7d6dfc6 to ce7677e Compare May 13, 2020 19:28

CaseyCarter added the enhancement Something can be improved label May 13, 2020

miscco force-pushed the equal_memcmp branch 6 times, most recently from 6ff69a1 to 1ab5481 Compare May 14, 2020 10:34

mnatsuhara reviewed May 14, 2020

View reviewed changes

[xutility] Modernize _Equal_memcmp_is_safe to use variable templates

9c64865

miscco force-pushed the equal_memcmp branch from 1ab5481 to 9c64865 Compare May 15, 2020 04:55

mnatsuhara approved these changes May 15, 2020

View reviewed changes

StephanTLavavej requested changes May 16, 2020

View reviewed changes

First batch of code review

8de249d

miscco force-pushed the equal_memcmp branch from 379c022 to 8de249d Compare May 16, 2020 11:36

miscco force-pushed the equal_memcmp branch from 8de249d to 9111fd7 Compare May 20, 2020 11:13

miscco force-pushed the equal_memcmp branch from 9111fd7 to 14d011d Compare May 20, 2020 17:42

miscco commented May 20, 2020

View reviewed changes

stl/inc/xutility Outdated Show resolved Hide resolved

tests/std/tests/VSO_0180469_ptr_cat/test.cpp Show resolved Hide resolved

Handle top level const properly

fe1a026

miscco force-pushed the equal_memcmp branch from 14d011d to fe1a026 Compare May 20, 2020 17:46

StephanTLavavej requested review from StephanTLavavej and mnatsuhara May 20, 2020 20:17

mnatsuhara reviewed May 21, 2020

View reviewed changes

stl/inc/xutility Outdated Show resolved Hide resolved

mnatsuhara approved these changes May 21, 2020

View reviewed changes

Use _Iter for iterators derp

12440c9

miscco added 2 commits May 23, 2020 12:57

Fix typos

c9b0e41

Add some more volatile tests with contiguous_iterators

5956df2

miscco force-pushed the equal_memcmp branch from 483a468 to 5956df2 Compare May 23, 2020 11:01

CaseyCarter approved these changes May 23, 2020

View reviewed changes

CaseyCarter linked an issue May 23, 2020 that may be closed by this pull request

<xutility>: Teach _Equal_memcmp_is_safe about contiguous iterators and ranges::equal_to #819

Closed

Adjust comments.

74c07f1

Simplify and comment metaprogramming.

bde44e4

Merge branch 'master' into equal_memcmp

31aea27

miscco commented May 29, 2020

View reviewed changes

stl/inc/xutility Show resolved Hide resolved

stl/inc/xutility Show resolved Hide resolved

StephanTLavavej requested changes May 29, 2020

View reviewed changes

StephanTLavavej added 2 commits May 29, 2020 01:52

Fix top-level constness on iterators, fix tests.

3390c01

Extend test coverage for element types.

58a8495

Include `<cstddef>` for `byte`. Test it when it's available. Style: Use `is_unsigned_v`. Add more test cases for mixed `int` and `long`. Remove a duplicated line (the cv pattern is clear).

StephanTLavavej approved these changes May 29, 2020

View reviewed changes

StephanTLavavej self-assigned this May 29, 2020

miscco commented May 29, 2020

View reviewed changes

tests/std/tests/VSO_0180469_ptr_cat/test.cpp Show resolved Hide resolved

Code review feedback: test byte == char.

be607e3

StephanTLavavej approved these changes May 29, 2020

View reviewed changes

CaseyCarter approved these changes May 29, 2020

View reviewed changes

stl/inc/xutility Show resolved Hide resolved

StephanTLavavej merged commit afaac16 into microsoft:master May 30, 2020

miscco deleted the equal_memcmp branch June 3, 2020 13:29

[xutility] Modernize _Equal_memcmp_is_safe to use variable templates #831

[xutility] Modernize _Equal_memcmp_is_safe to use variable templates #831

Uh oh!

Conversation

miscco commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miscco commented May 14, 2020

Uh oh!

mnatsuhara May 14, 2020

Choose a reason for hiding this comment

Uh oh!

miscco May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StephanTLavavej May 15, 2020

Choose a reason for hiding this comment

Uh oh!

miscco May 15, 2020

Choose a reason for hiding this comment

Uh oh!

miscco May 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miscco commented May 20, 2020

Uh oh!

StephanTLavavej commented May 20, 2020

Uh oh!

miscco commented May 20, 2020

Uh oh!

miscco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miscco commented May 23, 2020

Uh oh!

CaseyCarter commented May 23, 2020

Uh oh!

miscco commented May 28, 2020

Uh oh!

StephanTLavavej commented May 29, 2020

Uh oh!

miscco commented May 29, 2020

Uh oh!

miscco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

StephanTLavavej commented May 29, 2020

Uh oh!

Uh oh!

StephanTLavavej commented May 30, 2020

Uh oh!

miscco commented May 30, 2020

Uh oh!

cpplearner commented Nov 22, 2020

Uh oh!

AdamBucior commented Nov 22, 2020

miscco commented May 13, 2020 •

edited

Loading

miscco May 14, 2020 •

edited

Loading