- Notifications
You must be signed in to change notification settings - Fork1.6k
<type_traits>: is_function simplification#460
<type_traits>: is_function simplification#460StephanTLavavej merged 15 commits intomicrosoft:masterfrom
Conversation
CaseyCarter left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Did you intend to update the llvm-project submodule reference? (I suspect not.)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
ArtemSarmini commentedJan 25, 2020
I didn't, so llvm is reverted to original commit |
ArtemSarmini commentedJan 25, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Last commit added same optimization to |
StephanTLavavej commentedJan 25, 2020
I would say that the general rule is “follow the Standard’s order unless there’s a reason to do otherwise”. Type traits extensively use one another, so their order often varies from what’s depicted in the Standard, and that’s fine. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
ArtemSarmini commentedJan 26, 2020
|
BillyONeal commentedJan 27, 2020
Given that this is being billed as a throughput improvement can we get some form of benchmark demonstrating that this change is worth it? |
ArtemSarmini commentedJan 28, 2020
Will try to make one |
CaseyCarter left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've pushed a commit with a couple of formatting changes that were easier to make than comment. This otherwise seems ready to me.
StephanTLavavej commentedFeb 13, 2020
Regarding benchmarking, the undocumented compiler option |
StephanTLavavej commentedFeb 15, 2020
|
barcharcraz left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks fine to me, I did add comments about some comments (please feel free to comment about my comment comments), but given that they were probably per-existing, and also given that I don't actually care that much about them it's fine if you choose to ignore my suggestions.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
removed pointless comments, provided better _Arg_types description
barcharcraz commentedJul 7, 2020
I benched this with clang-cl and cl, As expected I saw very little change on CL. The testing program: #include<algorithm>#include<stdio.h>#include<vector>usingnamespacestd;intmain() { vector<int> v {1,7,2,9};sort(begin(v),end(v));} msvc before total: 0.110538s no change. However with clang-cl I saw more change I didn't rebase on master before testing the "after" changes, and I turned off warnings on the stl build (the compile options on the test program were identical. |
BillyONeal commentedJul 8, 2020
Thanks@barcharcraz :) |
* `is_member_function_pointer_v` should ignore cv-qualifiers (easy to fix) and calling conventions (super hard to fix). Bring back the inefficient `_Is_memfunptr` which is not so bad since (1) cl actually implements `is_member_function_pointer_v` in the compiler and (2) we can use an intrinsic for clang.Drive-by: Make `_Weak_types` an alias template.
StephanTLavavej left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There are several test failures, e.g.
"C:\agent\_work\1\s\tests\std\tests\Dev11_0535636_functional_overhaul\test.cpp",line 1219: error: static assertion failed with "TestRWTypes<Pmf0, float, X*, None, None>::value"STATIC_ASSERT(TestRWTypes<Pmf0, float, X*, None, None>::value);* `_Is_memfunptr` is sensitive to cv-qualifiers, they need to be stripped from its argument* `is_member_pointer_v` needs to work with calling conventions just like `is_member_function_pointer_v` does.
StephanTLavavej commentedJul 15, 2020
@BillyONeal is concerned that the benchmark might not have been exercising |
StephanTLavavej left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks! I apologize for my extreme delay in reviewing. I've looked through everything and it appears to be correct and simple, which is great. I noticed that the simplification could be extended tois_object_v; I'll validate this and then push a change.
Uh oh!
There was an error while loading.Please reload this page.
StephanTLavavej commentedAug 1, 2020
Thanks again for this simplification and compiler throughput improvement, and congratulations on your first microsoft/STL commit! This will ship in VS 2019 16.8 Preview 3. 🎉 😸 |
Uh oh!
There was an error while loading.Please reload this page.
Description
Closes#198. Implementation relies on
is_referenceandis_const, sois_functionwas moved down right afteris_volatile(I didn't want to separateis_constandis_volatile).Checklist
Be sure you've read README.md and understand the scope of this repo.
If you're unsure about a box, leave it unchecked. A maintainer will help you.
_Uglyas perhttps://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
verified by an STL maintainer before automated testing is enabled on GitHub,
leave this unchecked for initial submission).
members, adding virtual functions, changing whether a type is an aggregate
or trivially copyable, etc.).
the C++ Working Draft (including any cited standards), other WG21 papers
(excluding reference implementations outside of proposed standard wording),
and LWG issues as reference material. If they were derived from a project
that's already listed in NOTICE.txt, that's fine, but please mention it.
If they were derived from any other project (including Boost and libc++,
which are not yet listed in NOTICE.txt), youmust mention it here,
so we can determine whether the license is compatible and what else needs
to be done.