<div dir="rtl"><div dir="ltr">r245711, thanks!</div><div dir="ltr"><br></div></div><div class="gmail_extra"><br><div class="gmail_quote"><div dir="ltr">2015-08-21 20:19 GMT+03:00 Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span>:</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">LGTM too if you also remove the warning from the CodingStandards :).<br>
<div class="HOEnZb"><div class="h5"><br>
> On 2015-Aug-21, at 09:10, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br>
><br>
> LGTM! Thank you!<br>
><br>
> ~Aaron<br>
><br>
> On Fri, Aug 21, 2015 at 5:29 AM, Yaron Keren <<a href="mailto:yaron.keren@gmail.com">yaron.keren@gmail.com</a>> wrote:<br>
>> Is that OK?<br>
>><br>
>><br>
>> 2015-08-20 22:01 GMT+03:00 Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>>:<br>
>>><br>
>>> On Thu, Aug 20, 2015 at 2:28 PM, Duncan P. N. Exon Smith<br>
>>> <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
>>>><br>
>>>>> On 2015-Aug-20, at 10:53, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br>
>>>>><br>
>>>>> On Thu, Aug 20, 2015 at 1:49 PM, Yaron Keren <<a href="mailto:yaron.keren@gmail.com">yaron.keren@gmail.com</a>><br>
>>>>> wrote:<br>
>>>>>> So, there are three options<br>
>>>>>><br>
>>>>>> 1) Go _DEBUG_POINTER_IMPL route for VC2013 only. This will also<br>
>>>>>> disable the<br>
>>>>>> null checks in the other headers where they are beneficial and may<br>
>>>>>> miss real<br>
>>>>>> bugs.<br>
>>>>><br>
>>>>> If we limit it to just MSVC 2013, I think this is a reasonable route<br>
>>>>> to go because we can still get reasonable debug testing from MSVC 2015<br>
>>>>> users who build in debug mode. Then, when we drop support for 2013,<br>
>>>>> the check can go away.<br>
>>>>><br>
>>>>> ~Aaron<br>
>>>>><br>
>>>><br>
>>>> If it works, I like this best, but didn't you say that MSVC 2013 calls<br>
>>>> std::memcmp(nullptr) from std::equal()?  IIUC, then the interesting uses<br>
>>>> of std::equal() would invoke UB when compiled with MSVC 2013.  Whether<br>
>>>> or<br>
>>>> not it's a bug in MSVC, we shouldn't introduce UB :(.<br>
>>>><br>
>>>> Or did I miss a beat somewhere?<br>
>>><br>
>>> Only some of the overloads call std::memcmp(); if we are not using<br>
>>> std::equal() over container elements that are of type char, signed<br>
>>> char, or unsigned char, we should be fine. It seems those are the only<br>
>>> overloads for _Equal() (which is used to implement std::equal()) that<br>
>>> invoke std::memcmp().<br>
>>><br>
>>>><br>
>>>>>> 2) Replace std::equal with llvm::isEqual or stl::equal or ...<br>
>>>>>><br>
>>>>>> 3) Solve the llvm::equal ambiguity using namespace trick while making<br>
>>>>>> peace<br>
>>>>>> with the llvm::equal functor.<br>
>>>><br>
>>>> If #1 doesn't work, this is the option I like for the short-term, and<br>
>>>> we can remove the workaround (go back to std::equal()) when we drop<br>
>>>> support for MSVC 2013.<br>
>>><br>
>>> I would be fine with this as well if #1 cannot work out for some reason.<br>
>>><br>
>>> ~Aaron<br>
>>><br>
>>>><br>
>>>>>> 2015-08-18 16:13 GMT+03:00 Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>>:<br>
>>>>>>><br>
>>>>>>> On Mon, Aug 17, 2015 at 8:28 PM, Duncan P. N. Exon Smith<br>
>>>>>>> <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
>>>>>>>><br>
>>>>>>>>> On 2015-Aug-17, at 16:38, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>>>>>>>>> wrote:<br>
>>>>>>>>><br>
>>>>>>>>> On Mon, Aug 17, 2015 at 7:23 PM, Yaron Keren<br>
>>>>>>>>> <<a href="mailto:yaron.keren@gmail.com">yaron.keren@gmail.com</a>><br>
>>>>>>>>> wrote:<br>
>>>>>>>>>> An  issue that was patched several times in LLVM is<br>
>>>>>>>>>> std::equal(nullptr,<br>
>>>>>>>>>> nullptr, nullptr) where VC asserts on the third nullptr. Earlier<br>
>>>>>>>>>> in<br>
>>>>>>>>>> the<br>
>>>>>>>>>> thread Marshal wrote this should return true and not assert.<br>
>>>>>>>>><br>
>>>>>>>>> Yes, I've fixed some myself, IIRC. Marshall's logic makes sense to<br>
>>>>>>>>> me,<br>
>>>>>>>>> and from my reading of the standard, it also seems to fit.<br>
>>>>>>>><br>
>>>>>>>> 25.2.11 is pretty clear:<br>
>>>>>>>><br>
>>>>>>>>> Returns: true if for every iterator i in the range [first1,last1)<br>
>>>>>>>>> the<br>
>>>>>>>>> following corresponding condi- tions hold: *i == *(first2 + (i -<br>
>>>>>>>>> first1)),<br>
>>>>>>>>> pred(*i, *(first2 + (i - first1))) != false. Otherwise, returns<br>
>>>>>>>>> false.<br>
>>>>>>>><br>
>>>>>>>><br>
>>>>>>>> Unless you're suggesting that `[nullptr, nullptr)` is an invalid<br>
>>>>>>>> iterator range, or that `nullptr` is an invalid iterator?  But<br>
>>>>>>>> `[nullptr, nullptr)` supports all the necessary operations for an<br>
>>>>>>>> empty iterator range, so how could that be?  (Maybe I'm guessing<br>
>>>>>>>> wrong and this is a straw man... in which case, how could this be<br>
>>>>>>>> a conforming extension?)<br>
>>>>>>><br>
>>>>>>> I took longer to read more of the standard (specifically, the parts<br>
>>>>>>> about library requirements in [res.on.arguments] and [iterators]),<br>
>>>>>>> and<br>
>>>>>>> I don't think this is a conforming extension. Then I fired up MSVC<br>
>>>>>>> 2015 and did some experiments, and realized the behavior is different<br>
>>>>>>> in 2015, so I'm not even certain the behavior was intentional in the<br>
>>>>>>> first place.<br>
>>>>>>><br>
>>>>>>> In MSVC 2013, equal would check the range and then check the last<br>
>>>>>>> pointer for null, and this check would trigger the assertion.<br>
>>>>>>> In MSVC 2015, equal would check the range, and then check the last<br>
>>>>>>> pointer for null only if the range is not empty, and so no assertion<br>
>>>>>>> is triggered.<br>
>>>>>>><br>
>>>>>>> (There is still a degenerate case bug where std::equal(nullptr,<br>
>>>>>>> nullptr, nullptr) refuses to compile that I've reported.)<br>
>>>>>>><br>
>>>>>>>>> However,<br>
>>>>>>>>> I'm not convinced this isn't a valid extension that prevents UB<br>
>>>>>>>>> from<br>
>>>>>>>>> calling std::memcmp() with a null pointer value.<br>
>>>>>>>><br>
>>>>>>>> `std::equal` isn't defined in terms of `std::memcmp()`, it's defined<br>
>>>>>>>> as above.  How could it be valid to return anything but `true`?  The<br>
>>>>>>>> wording is clear.<br>
>>>>>>>><br>
>>>>>>>> But if MSVC is calling `std::memcmp()` directly then we probably<br>
>>>>>>>> don't have a choice but to reimplement `std::equal()` ourselves,<br>
>>>>>>>> correctly :(.<br>
>>>>>>><br>
>>>>>>> I think that's true in MSVC 2013, unfortunately. However, I also<br>
>>>>>>> think<br>
>>>>>>> that speaks to a hackish solution limited to just MSVC 2013 being<br>
>>>>>>> somewhat more acceptable because we will be getting rid of 2013<br>
>>>>>>> sometime in the future (next few years).<br>
>>>>>>><br>
>>>>>>> ~Aaron<br>
>>>>>>><br>
>>>>>>>><br>
>>>>>>>>> I think LWG would<br>
>>>>>>>>> need to weigh in on the matter, but I don't know of any issues that<br>
>>>>>>>>> are open on this topic that we could point to for a definitive<br>
>>>>>>>>> answer.<br>
>>>>>>>>> Perhaps Marshall is aware of something, however.<br>
>>>>>>>><br>
>>>>>>>><br>
>>>>>><br>
>>>>>><br>
>>>><br>
>><br>
>><br>
<br>
</div></div></blockquote></div><br></div>