<type_traits>: is_function simplification

[ArtemSarmini]

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 _Ugly as 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.

[CaseyCarter]

Did you intend to update the llvm-project submodule reference? (I suspect not.)

[ArtemSarmini]

I didn't, so llvm is reverted to original commit

[ArtemSarmini]

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?

[StephanTLavavej]

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.

[ArtemSarmini]

is_object_v uses is_function_v, so it was moved down.

[BillyONeal]

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]

Will try to make one

[CaseyCarter]

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]

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

[StephanTLavavej]

/d1reportTimeSummary can also be used to benchmark this change.

[barcharcraz]

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.

[barcharcraz]

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>

using namespace std;

int main() {
    vector<int> 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.

[BillyONeal]

Thanks @barcharcraz :)

[StephanTLavavej]

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);

[StephanTLavavej]

@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.)

[StephanTLavavej]

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.

[StephanTLavavej]

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. 🎉 😸