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

[Eric Fiselier via cfe-commits]

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.
>>>
>>>
>>
>