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

[Richard Smith via cfe-commits]

On Fri, Oct 9, 2015 at 3: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.
>

OK, I'll include a different header instead.

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)?
>

libc++ provides static_assert emulation in the cases where it's not
available, and other tests are using it unconditionally.


> 3. Could you throw the standarese that requires this behavior at the
> top of the test?
>

Done.


> LGTM after you address those points.


Thanks, all done in r249931 (other than the one reverted patch).

/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.
> >>>>
> >>>>
> >>>
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151009/74503ba7/attachment-0001.html>