[PATCH] D12747: Implement [depr.c.headers]

[Eric Fiselier via cfe-commits]

@Marshall, @Richard Have we fixed the Solaris build yet?

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