Add missing deduction guides and use type_identity_t for allocator extended copy/move ctors
#556
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #556 +/- ##
=======================================
Coverage 99.60% 99.60%
=======================================
Files 11 11
Lines 750 750
Branches 76 76
=======================================
Hits 747 747
Misses 3 3 ☔ View full report in Codecov by Sentry. |
| @@ -13,6 +13,7 @@ cc_library( | |||
| srcs = ["indirect.cc"], | |||
| hdrs = ["indirect.h"], | |||
| copts = ["-Iexternal/value_types/"], | |||
| defines = ["XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION"], | |||
There was a problem hiding this comment.
What is the purpose of XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION? Does it cause problems for some compilers?
I would expect that all compilers should be able to support these deduction guides, and so you wouldn't need to hide them under a build flag.
There was a problem hiding this comment.
I'd like to keep extended features as explicit opt-in for now.
We check XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION in tests to ensure that appropriate tests are only run when extended features are enabled.
| // | ||
| // XYZ_HAS_STD_TYPE_IDENTITY | ||
| // The macro is defined when std::type_identity_t<T> is available. | ||
| // | ||
|
|
||
| #ifdef XYZ_HAS_STD_TYPE_IDENTITY | ||
| #error "XYZ_HAS_STD_TYPE_IDENTITY is already defined" | ||
| #endif // XYZ_HAS_STD_TYPE_IDENTITY | ||
|
|
||
| #if (__cplusplus >= 202002L) || (defined(_MSVC_LANG) && _MSVC_LANG >= 202002L) | ||
| #define XYZ_HAS_STD_TYPE_IDENTITY | ||
| #endif //(__cplusplus >= 202002L) || (defined(_MSVC_LANG) && _MSVC_LANG >= | ||
| // 202002L) |
There was a problem hiding this comment.
This macro is set but never used. You use type_identity_t below without testing this macro.
There was a problem hiding this comment.
It's used in tests to disable tests that won't run on older C++ versions.
There was a problem hiding this comment.
Yeah, but it's not used to guard those features in indirect.h. So for example this just errors out:
clang++ -O2 -std=c++17 -c indirect_test.cc
Theoretically you could support CTAD in C++17 (because CTAD itself was a C++17 feature); but right now you have only your C++14 version (which can't use CTAD) and your C++20 version (which assumes full C++20 support).
I'm figuring all this out as I write, so, forgive me if this is all old news. I guess the README already implies that C++17 isn't a supported target. You can probably at least partially ignore this comment, then.
I'm still confused why XYZ_HAS_EXTENDED_CONSTRUCTOR_TEMPLATE_ARGUMENT_DEDUCTION is handled/set/tested in a different way from XYZ_HAS_TEMPLATE_ARGUMENT_DEDUCTION, though.
| template <typename Alloc, typename Value, | ||
| typename std::enable_if_t<!is_indirect_v<Value>, int> = 0> |
There was a problem hiding this comment.
| template <typename Alloc, typename Value, | |
| typename std::enable_if_t<!is_indirect_v<Value>, int> = 0> | |
| template <typename Alloc, typename Value> |
The enable_if isn't needed, since the deduction guide on line 475 is more specific. (Please correct me if I'm wrong; I didn't test this.)
There was a problem hiding this comment.
I've taken another look and the enable_if is needed for AppleClang at least.
This issue is possibly related: llvm/llvm-project#57646
Perhaps @Ukilele can comment as he'd had some success implementing deduction guides.
| template <typename Alloc, typename Value, | ||
| typename std::enable_if_t<!is_polymorphic_v<Value>, int> = 0> |
There was a problem hiding this comment.
| template <typename Alloc, typename Value, | |
| typename std::enable_if_t<!is_polymorphic_v<Value>, int> = 0> | |
| template <typename Alloc, typename Value> |
Same here: the deduction guide on line 427 is more specific so the enable_if isn't needed.
And a big bonus here: you can get rid of your xyz::is_polymorphic_v which is confusingly dissimilar to std::is_polymorphic_v.
There was a problem hiding this comment.
std::is_polymorphic_v<std::polymorphic<T>> evaluating to false is a pretty compelling call for a rename. A new issue shall be raised.
There was a problem hiding this comment.
FYI, std::is_array_v<std::array<int, 10>> and std::is_function_v<std::function<int()>> are both false too. (My C++ Pub Quiz research is finally paying off! ;)) I think it's a bonus to be able to eliminate xyz::is_polymorphic, but if it's really indispensable, then I don't think you should worry about changing the name.
Enable more use of deduction guides.