From 6078246acdb8784febc023d5d9e0fb4ddcd81f34 Mon Sep 17 00:00:00 2001 From: Phil Willoughby Date: Thu, 17 Aug 2017 01:27:07 -0700 Subject: [PATCH] Remove a use of SFINAE in the Range constructor. Summary: Specifically, the constructor `implicit Range(Iter)` is now declared and defined for all `Range` specializations. It is an error to use it, and we `static_assert`, unless `Iter` is `char const *` or `char *`. Performance effect --- Measuring compilation-time on a file that just make ~40k StringPieces, compiled with -O3. All compilers produced identical code before/after this change. * clang-trunk: 4% improvement * gcc-5: no change * gcc-4.9: 11% improvement * gcc-7.1: 5% improvement Could this possibly break any existing code? --- Yes. If you have a function that's overloaded for both `Range` and `Range` and your input object is not `char const*` and is implicitly convertible to both `char const*` and `X`: that is now ambiguous whereas before it would unambiguously pick the `Range` overload. I don't consider this scenario likely. Why should this work? --- Using SFINAE is more expensive at compile time (with some compilation environments) than not using it. It's necessary to use SFINAE when there is an alternative function which will be used in preference when the substitution fails, but when that is not the case it is on average cheaper to make the function always exist and use static_assert to disallow the bad uses of it. A bonus is that the caller gets a more comprehensible error message. Reviewed By: nbronson Differential Revision: D5639502 fbshipit-source-id: 13469f2995a487398734f86108087fdc8e32ad71 --- folly/Range.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/folly/Range.h b/folly/Range.h index 8ae96ce6..d1fa497c 100644 --- a/folly/Range.h +++ b/folly/Range.h @@ -212,9 +212,12 @@ class Range : private boost::totally_ordered> { /* implicit */ Range(std::nullptr_t) = delete; #endif - template ::type = 0> constexpr /* implicit */ Range(Iter str) - : b_(str), e_(str + constexpr_strlen(str)) {} + : b_(str), e_(str + constexpr_strlen(str)) { + static_assert( + std::is_same::type>::value, + "This constructor is only available for character ranges"); + } template ::const_type = 0> /* implicit */ Range(const std::string& str) -- 2.34.1