This patch adds a new trait to allow standard libraries to forward std::equal calls to memcmp in more cases.
Hmmm, is this effectively std::has_unique_object_representations (ensuring that all bits of the object representation contribute to the value)?
Dependent types should probably return false?
I think this might be missing cases, like complex integers, scoped enumerations, vector types?, pointer types, atomic versions of these types...
These two traits are indeed very similar. The main difference is that has_unique_object_representations requires that the type is trivially copyable, and __is_trivially_equality_comparable requires a defaulted equality comparison operator.
I think this should be covered by the switch to using hasUniqueObjectRepresentation.
Ah, I like this approach -- it keeps things roughly in sync with checking for unique object representations, which is great. I spotted a few questions, but in general this is heading in the right direction. You should also add a release note to clang/docs/ReleaseNotes.rst so that users know there's a new builtin coming.
I suppose one thing to double-check: do you know of any other STL implementations that might be using this identifier as part of their implementation details? (We've had issues in the past where we accidentally stole a reserved identifier that was being used by libstdc++ and it caused issues.)
Spurious whitespace change.
Hmmm, is this correct? I thought there was a defaulted equality comparison operator in this case, but it's defined as deleted.
Perhaps this function is looking for a usable defaulted equality comparison operator and not just "does it have one at all"?
Do we have to look for fields with references per http://eel.is/c++draft/class.compare.default#2 ?
Spurious whitespace changes.
Neither libstdc++ nor the MSVC STL use the __is_trivially_equality_comparable AFAICT. A sourcegraph search also exclusively finds libc++: https://sourcegraph.com/search?q=context%3Aglobal+__is_trivially_equality_comparable&patternType=standard&sm=1&groupBy=repo (which only started using it in this release cycle, so it shouldn't be a problem).
Yes, this is looking for a usable one. I've renamed it. (Maybe someone has an idea for a better name?)
Perfect, thank you for double-checking!
One thing I can't quite determine is whether we expect to call this type trait often or not. Are either of the uses in libc++ going to be instantiated frequently? I'm trying to figure out whether we want the current implementation of HasNonDeletedDefaultedEqualityComparison() or whether we should bite the bullet up front and add another bit to CXXRecordDecl::DefinitionData so we compute this property once as the class is being formed and don't have to loop over all of the methods and bases each time. We do this for other special member functions, as with: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/DeclCXX.h#L695
Why the check for !RecordType? If any field is a reference, we can bail out early, so I think this is better as:
if (FD->getType()->isReferenceType()) return false; if (const auto *RD = FD->getType()->getAsCXXRecordDecl()) return HasNonDeletedDefaultedEqualityComparison(RD); return true;
Avoids calling the same method twice and avoids using a type name as a declaration identifier.
We currently use __is_trivially_equality_comparable only for std::equal (and I'm working on using it for std::find), and I don't think there are a lot more places where we can use it. It's also only relevant for cases where we have raw pointers. Would the enable_if be evaluated, even if the arguments aren't raw pointers in the example below?
template <class T, class U, enable_if_t<is_same_v<remove_cvref_t<T>, remove_cvref_t<U>> && __is_trivially_equality_comparable(T)>> bool equal(T* first1, T* last1, U* first2); <non-raw-pointer-overload>
If not, then I don't think it makes sense to save the information, since it will most likely not be evaluated more than once or twice per type.
I don't really have an opinion here. Your version seems reasonable, so I'm going to take it.
I mainly glossed over the patch and didn't do a real review.
|2672 ↗||(On Diff #511762)|
This does not magically use the builtin right? Does the patch miss that parts that use the builtin or is that a followup? If it's a followup I would prefer move the libc++ code changes of this patch to a separate review.
No, it doesn't magically use the builtin. I plan to do that in a follow-up. These changes are required though, since clang grabs the name with this patch.
But it would still be possible to have these libc++ changes in a separate review and commit them separately. I would say the libc++ changes are quite trivial and can be landed quite quickly.
I believe that we deduce the type of T before we can instantiate any of the deduced template parameters, so I think the enable_if is not evaluated unless T is known to have a raw pointer type. Based on that, I don't think we need to steal any bits anywhere right now -- we can use the current approach and if it turns up on the hot path at some point, we can address the concern then. The clang bits LGTM, but I'll hold off on signing off until someone from libc++ approves as well.
We have a setup in which clang (more specifically clang-tools) is always built from a version close to HEAD and libcxx is fetched from user's checkout of the codebase (which can lag behind HEAD for ~a month).
So the 1 week gap between https://github.com/llvm/llvm-project/commit/380b6a13dad61e1d90e68fdd128af5dc177db3e1 and this patch is not enough for us to prevent most of the breakages (clang-tools we have cannot work on checkouts that are not synced past this commit as they hit the name conflict).
Hence is it possible to revert this change and land it couple weeks later or add the token behind a PP-directive (looking at the way token is introduced having this optionally available seems to be hard though)?
also it would be nice to have general policies about changes that might break backward compat and how long to wait (i guess we would be more careful if the libcxx symbol was actually used in a released version). do we already have any guidance around that so that we can be prepared for those in the future?