ADL-proof container operations for iterator pairs and comparators #4258
Conversation
| _Insert_counted_range(_Where, _UFirst, _Count); | ||
| #ifdef __cpp_lib_concepts | ||
| } else if constexpr (forward_iterator<_Iter>) { | ||
| const auto _Length = _To_unsigned_like(_RANGES distance(_UFirst, _ULast)); | ||
| const auto _Length = _STD _To_unsigned_like(_RANGES distance(_UFirst, _ULast)); | ||
| const auto _Count = _Convert_size<size_type>(_Length); |
There was a problem hiding this comment.
Should this line also be changed?
Regex-searching for (?<!_STD )(_Adl_verify_range|_Get_unwrapped|_Convert_size|_To_unsigned_like|_Allocate_at_least_helper|_Uninitialized_\w+|_Destroy_range|_Move_backward_unchecked|_Copy_n_unchecked4), I see 75 unchanged occurrences in <vector>. Some I understand (e.g. an initializer_list always has size_t for its size(), so we don't need to worry about ADL), but I don't understand why others aren't being changed.
Since a bunch of lines are potentially affected and I don't totally understand what's happening here, I won't push changes. We can deal with this in a followup.
There was a problem hiding this comment.
The result of _To_unsigned_like is always an integer-like type. It either has no associated namespaces - if it's integral - or we control the associated namespaces - if it's integer-class. (Recall that integer-like types are either integral or integer-class, and that integer-class types are only implementation-provided.)
I'm concerned that we seem to be developing more and more complex rules to determine when to _STD-qualify calls; maybe we should just qualify everything? (I think I may have started this by not qualifying _Ubegin and _Uend calls.)
There was a problem hiding this comment.
BTW I was asking about qualifying _Convert_size here, since the occurrence immediately above was changed.
At this point I think we should just qualify everything, if we want to defend against this ADL thing. It's too hard to enforce consistency otherwise. Same rationale as _STD qualifying all pretty calls regardless of arguments or lack thereof.
There was a problem hiding this comment.
BTW I was asking about qualifying
_Convert_sizehere, since the occurrence immediately above was changed.
Yes - I understood this, I was explaining that we don't need to qualify the _Convert_size call since we control the argument type and can ensure that ADL is "safe".
At this point I think we should just qualify everything, if we want to defend against this ADL thing. It's too hard to enforce consistency otherwise. Same rationale as
_STDqualifying all pretty calls regardless of arguments or lack thereof.
+1 from me.
| T another(begin(c), end(c)); | ||
| T another2(begin(c), end(c)); | ||
| T another2(begin(c), end(c), value.get_allocator()); |
There was a problem hiding this comment.
No change requested: Nice catch!
tests/std/tests/VSO_0000000_instantiate_containers/test.compile.pass.cpp
Outdated
Show resolved
Hide resolved
|
Thanks! I pushed a conflict-free merge with I have one outstanding question but it shouldn't block this PR. |
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
😸 📦 🛡️ |
Towards #140. Some changes were already done in #4217.
Unblocks one libcxx test:
std/strings/basic.string/string.modifiers/robust_against_adl.pass.cppNotes:
_Adl_verify_rangeare consistently_STD-qualified in touched headers.