: is_function simplification by ArtemSarmini · Pull Request #460 · microsoft/STL (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation46 Commits15 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Description
Closes #198. Implementation relies on is_reference and is_const, so is_function was moved down right after is_volatile (I didn't want to separate is_const and is_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.
- Identifiers in product code changes are properly
_Uglyas per
https://eel.is/c++draft/lex.name#3.1 or there are no product code changes. - The STL builds successfully and all tests have passed (must be manually
verified by an STL maintainer before automated testing is enabled on GitHub,
leave this unchecked for initial submission). - These changes introduce no known ABI breaks (adding members, renaming
members, adding virtual functions, changing whether a type is an aggregate
or trivially copyable, etc.). - These changes were written from scratch using only this repository,
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), you must mention it here,
so we can determine whether the license is compatible and what else needs
to be done.
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.)
I didn't, so llvm is reverted to original commit
Last commit added same optimization to is_member_function_pointer and is_member_object_pointer. I had to move stuff around the file a lot, which is bad, as I heard we tend to have the same declaration order as standard. Maybe _Implementation_details should be defined first in another header like xtraits?
I had to move stuff around the file a lot, which is bad, as I heard we tend to have the same declaration order as standard.
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.
is_object_v uses is_function_v, so it was moved down.
Given that this is being billed as a throughput improvement can we get some form of benchmark demonstrating that this change is worth it?
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.
Regarding benchmarking, the undocumented compiler option /d1templateStats may provide sufficient evidence. Here's an example:
C:\Temp>type meow.cpp
#include <algorithm>
#include <stdio.h>
#include <vector>
using namespace std;
int main() {
vector<int> v{1, 7, 2, 9};
sort(v.begin(), v.end());
for (const auto& e : v) {
printf("%d, ", e);
}
printf("\n");
}
C:\Temp>cl /EHsc /nologo /W4 /MT /d1templateStats meow.cpp
meow.cpp
Total templates : 371
Class templates : 255
Class template specializations : 56
Class template partial specializations : 200
Class template explicit specializations : 55
Class template tentative specializations : 573
Total class template specializations : 884
Most class template specializations : "__vcrt_va_list_is_reference" (5)
Most class template partial specializations : "std::_Is_function" (36)
Most class template explicit specializations : "std::numeric_limits" (18)
Most class template tentative specializations: "std::integral_constant" (49)
Function templates : 49
Function template specializations : 61
Function template explicit specializations : 1
Total function template specializations : 62
Most function specializations : "std::declval" (6)
Most function explicit specializations : "std::_Convert_size" (1)
Alias templates : 43
Alias template specializations : 242
Most alias template specializations : "std::void_t" (43)
Variable templates : 24
Variable template specializations : 49
Variable template partial specializations : 4
Variable template explicit specializations : 0
Variable template tentative specializations : 0
Most variable template specializations : "std::is_same_v" (9)
Reparsed function template specializations : 61
Substituted function template specializations : 0
/d1reportTimeSummary can also be used to benchmark this change.
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.
removed pointless comments, provided better _Arg_types description
I benched this with clang-cl and cl, As expected I saw very little change on CL.
The testing program:
#include #include <stdio.h> #include
using namespace std;
int main() { vector v {1,7,2,9}; sort(begin(v), end(v)); }
msvc before total: 0.110538s
msvc after total: 0.110835s
no change.
However with clang-cl I saw more change
clang-cl before: 451.931 ms
clang-cl after: 187.441 ms
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.
is_member_function_pointer_vshould ignore cv-qualifiers (easy to fix) and calling conventions (super hard to fix). Bring back the inefficient_Is_memfunptrwhich is not so bad since (1) cl actually implementsis_member_function_pointer_vin the compiler and (2) we can use an intrinsic for clang.
Drive-by: Make _Weak_types an alias template.
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_memfunptris sensitive to cv-qualifiers, they need to be stripped from its argumentis_member_pointer_vneeds to work with calling conventions just likeis_member_function_pointer_vdoes.
@BillyONeal is concerned that the benchmark might not have been exercising is_function at all, and that executable startup times may have been producing misleading results. (sort doesn't; it just invokes whatever function object it's been given.)
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 to is_object_v; I'll validate this and then push a change.
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. 🎉 😸