On Fri, Oct 9, 2015 at 3:48 PM, Eric Fiselier <e...@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 <e...@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 <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