<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Oct 9, 2015 at 3:48 PM, Eric Fiselier <span dir="ltr"><<a href="mailto:eric@efcs.ca" target="_blank">eric@efcs.ca</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Regarding Patch #15.<br>
<br>
1. Tests under 'test/std' shouldn't directly include <__config> or<br>
depend on any libc++ implementation details. We are trying to make the<br>
test suite generic so refrain from referencing libc++ symbols.<br></blockquote><div><br></div><div>OK, I'll include a different header instead.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
2. "static_assert" is C++11 only but this test should work in C++03.<br>
Can you use "#if TEST_STD_VER >= 11" from "test_macros.h" to use<br>
static assert in C++11 and just "assert" in C++03 (or something<br>
similar)?<br></blockquote><div><br></div><div>libc++ provides static_assert emulation in the cases where it's not available, and other tests are using it unconditionally.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
3. Could you throw the standarese that requires this behavior at the<br>
top of the test?<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
LGTM after you address those points.</blockquote><div><br></div><div>Thanks, all done in r249931 (other than the one reverted patch).</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span><font color="#888888">
/Eric<br>
</font></span><div><div><br>
<br>
On Fri, Oct 9, 2015 at 4:26 PM, Eric Fiselier <<a href="mailto:eric@efcs.ca" target="_blank">eric@efcs.ca</a>> wrote:<br>
> Patch #12 LGTM. Thanks for doing tho cwchar approach in this patch.<br>
> One small nit. I would prefer a "negative" feature macro for<br>
> "_LIBCPP_STRING_H_HAS_CONST_OVERLOADS" because correct defaults<br>
> shouldn't need a macro definition to be selected. (ie<br>
> _LIBCPP_STRING_H_HAS_NO_CONST_OVERLOAD.)<br>
><br>
> /Eric<br>
><br>
> On Fri, Oct 9, 2015 at 1:59 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
>> As of r249890, all committed other than patches 12 (string.h) and 15 (more<br>
>> tests).<br>
>><br>
>> On Thu, Oct 8, 2015 at 9:12 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
>>><br>
>>> On Thu, Oct 8, 2015 at 6:58 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>><br>
>>> wrote:<br>
>>>><br>
>>>> On Thu, Oct 8, 2015 at 6:25 PM, Eric Fiselier <<a href="mailto:eric@efcs.ca" target="_blank">eric@efcs.ca</a>> wrote:<br>
>>>>><br>
>>>>> Patch #12 needs revision. A bunch of function definitions were moved<br>
>>>>> out of the std namespace and into the global.<br>
>>>>> That change is incorrect.<br>
>>>><br>
>>>><br>
>>>> Slightly updated version attached. I should probably explain what's going<br>
>>>> on here in more detail:<br>
>>>><br>
>>>> Per [c.strings]p4-p13, we're supposed to replace C functions of the form<br>
>>>><br>
>>>>   char *foo(const char*);<br>
>>>><br>
>>>> with a pair of const-correct overloads of the form<br>
>>>><br>
>>>>   char *foo(char *);<br>
>>>>   const char *foo(const char*);<br>
>>>><br>
>>>> Now, most C standard libraries will do this for us when included in C++<br>
>>>> mode (because it's not possible for the C++ library to fix this up after the<br>
>>>> fact). For the cases where we *don't* believe we have such a considerate C<br>
>>>> library, we add one declaration to C's overload, to get:<br>
>>>><br>
>>>>   char *foo(char*);<br>
>>>>   char *foo(const char*)<br>
>>>><br>
>>>> ... which doesn't really help much, but is the closest we can get to the<br>
>>>> right set of declarations. The declarations we add just dispatch to the C<br>
>>>> declarations.<br>
>>>><br>
>>>> These new declarations *should* be in the global namespace when including<br>
>>>> <string.h>, and it makes sense for us to put them in the global namespace<br>
>>>> when including <cstring> (otherwise, that header leaves us with a broken<br>
>>>> overload set in the global namespace, containing just one of the two<br>
>>>> expected functions).<br>
>>>><br>
>>>> Anyway, most of the above is a description of what we did before. What's<br>
>>>> new here is that we attempt to fix the overload set for both <string.h> and<br>
>>>> for <cstring>, not just for the latter. At the end of all these changes,<br>
>>>> you'll see that all that the <cfoo> headers do is to include the <foo.h><br>
>>>> header and use using-declarations to pull the names into namespace std; this<br>
>>>> is no exception to that pattern.<br>
>>><br>
>>><br>
>>> Per Eric and my discussion on IRC, the pattern used by <cwchar> seems<br>
>>> better here:<br>
>>><br>
>>> If libc has left us with a bad overload set, don't try to fix the names in<br>
>>> ::, just provide a complete set of overloads in namespace std.<br>
>>><br>
>>> A patch for that approach is attached.<br>
>>><br>
>>>>> On Thu, Oct 8, 2015 at 7:09 PM, Eric Fiselier <<a href="mailto:eric@efcs.ca" target="_blank">eric@efcs.ca</a>> wrote:<br>
>>>>> > Patch #11 LGTM. Any reason you removed the "#pragma diagnostic ignored<br>
>>>>> > "-Wnonnull"" in test/std/depr/depr.c.headers/stdlib_h.pass.cpp?<br>
>>>>> > I would like to leave it in so this test doesn't fail with older clang<br>
>>>>> > versions.<br>
>>>>> ><br>
>>>>> > /Eric<br>
>>>>> ><br>
>>>>> > On Thu, Oct 8, 2015 at 6:47 PM, Eric Fiselier <<a href="mailto:eric@efcs.ca" target="_blank">eric@efcs.ca</a>> wrote:<br>
>>>>> >> Patch #10 LGTM.<br>
>>>>> >><br>
>>>>> >> On Thu, Oct 8, 2015 at 4:28 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>><br>
>>>>> >> wrote:<br>
>>>>> >>> On Thu, Oct 8, 2015 at 11:50 AM, Marshall Clow<br>
>>>>> >>> <<a href="mailto:mclow.lists@gmail.com" target="_blank">mclow.lists@gmail.com</a>><br>
>>>>> >>> wrote:<br>
>>>>> >>>><br>
>>>>> >>>> On Tue, Oct 6, 2015 at 3:57 PM, Richard Smith<br>
>>>>> >>>> <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>><br>
>>>>> >>>> wrote:<br>
>>>>> >>>>><br>
>>>>> >>>>> <stddef.h>. This one is tricky:<br>
>>>>> >>>>><br>
>>>>> >>>>> 1) There's an (undocumented) interface between the C standard<br>
>>>>> >>>>> library and<br>
>>>>> >>>>> this header, where the macros __need_ptrdiff_t, __need_size_t,<br>
>>>>> >>>>> __need_wchar_t, __need_NULL, __need_wint_t request just a piece of<br>
>>>>> >>>>> this<br>
>>>>> >>>>> header rather than the whole thing. If we see any of those, just<br>
>>>>> >>>>> go straight<br>
>>>>> >>>>> to the underlying header.<br>
>>>>> >>>><br>
>>>>> >>>><br>
>>>>> >>>> Ok, but in that case we don't get nullptr.  I suspect that's OK.<br>
>>>>> >>>><br>
>>>>> >>>>><br>
>>>>> >>>>> 2) We probably don't want <stddef.h> to include <cstddef> (for<br>
>>>>> >>>>> consistency with other headers)<br>
>>>>> >>>><br>
>>>>> >>>><br>
>>>>> >>>> No, we do not! :-)<br>
>>>>> >>>><br>
>>>>> >>>>><br>
>>>>> >>>>> , but <stddef.h> must provide a ::nullptr_t (which we don't want<br>
>>>>> >>>>> <cstddef> to provide). So neither header includes the other.<br>
>>>>> >>>>> Instead, both<br>
>>>>> >>>>> include <__nullptr> for std::nullptr_t, and we duplicate the<br>
>>>>> >>>>> definition of<br>
>>>>> >>>>> max_align_t between them, in the case where the compiler's<br>
>>>>> >>>>> <stddef.h><br>
>>>>> >>>>> doesn't provide it.<br>
>>>>> >>>>><br>
>>>>> >>>>> If you prefer, I could make <stddef.h> include <cstddef> to avoid<br>
>>>>> >>>>> the<br>
>>>>> >>>>> duplication of the max_align_t logic.<br>
>>>>> >>>><br>
>>>>> >>>><br>
>>>>> >>>> No; this is a minor annoyance, and layer jumping (<stdXXX.h><br>
>>>>> >>>> including<br>
>>>>> >>>> <cstdXXX>) is a major annoyance - and I'm pretty sure that that<br>
>>>>> >>>> would come<br>
>>>>> >>>> back to bite us in the future.<br>
>>>>> >>>><br>
>>>>> >>>> Looks ok to me.<br>
>>>>> >>><br>
>>>>> >>><br>
>>>>> >>> Thanks, everything up to and including patch 09 is now committed.<br>
>>>><br>
>>>><br>
>>><br>
>><br>
</div></div></blockquote></div><br></div></div>