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 <rich...@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 <rich...@metafoo.co.uk> wrote: >> >> On Thu, Oct 8, 2015 at 6:58 PM, Richard Smith <rich...@metafoo.co.uk> >> wrote: >>> >>> On Thu, Oct 8, 2015 at 6:25 PM, Eric Fiselier <e...@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 <e...@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 <e...@efcs.ca> wrote: >>>> >> Patch #10 LGTM. >>>> >> >>>> >> On Thu, Oct 8, 2015 at 4:28 PM, Richard Smith <rich...@metafoo.co.uk> >>>> >> wrote: >>>> >>> On Thu, Oct 8, 2015 at 11:50 AM, Marshall Clow >>>> >>> <mclow.li...@gmail.com> >>>> >>> wrote: >>>> >>>> >>>> >>>> On Tue, Oct 6, 2015 at 3:57 PM, Richard Smith >>>> >>>> <rich...@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. >>> >>> >> > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits