Thanks Jorge. I'll work on committing this today. -- HT
On Fri, Feb 12, 2016 at 12:10 AM, Jorge Teixeira <j.lopes.teixe...@gmail.com > wrote: > Hubert, > > Thanks for the code review. Over the weekend I'll try to learn a bit > more about using Phabricator, but for now I'll reply here, and attach > a new patch. > > a) *_MANT_DIG < 1 --> *_MANT_DIG < 2 > That is a stricter check and I agree with your rationale. Done. > > b) _MIN_EXP --> FLT_MIN_EXP > Done. > > c) Remove _MIN_EXP and _MIN_10_EXP FLT,DBL,LDBL comparisons > Yes, as you and Richard pointed out the added mantissa bits can > compensate for the lack of increase of the exponent. > Already fixed in http://reviews.llvm.org/rL260639. > > d) *_MAX_EXP and *_MIN_EXP 2,-2 --> 1,-1 > Done. > > Richard, will do re: single patch for multiple files. Also, can you > close the bug report? Even if more tests for float.h get > added/changed, the original problem has been solved. > > JT > > > On Thu, Feb 11, 2016 at 8:38 PM, Hubert Tong > <hubert.reinterpretc...@gmail.com> wrote: > > Hi Jorge, > > > > I responded to the initial commit with some comments here: > > http://reviews.llvm.org/rL260577 > > > > -- HT > > > > On Thu, Feb 11, 2016 at 7:53 PM, Jorge Teixeira < > j.lopes.teixe...@gmail.com> > > wrote: > >> > >> > You'll also need to change <float.h> to only provide DECIMAL_DIG in > C99 > >> > onwards. > >> Done! > >> > >> > All of our -std versions are that standard plus applicable Defect > >> > Reports. So -std=c89 includes TC1 and TC2, but not Amendment 1 (we > >> > have -std=c94 for that, but the only difference from our C89 mode is > >> > the addition of digraphs). > >> I'll try to find the c89 TC2 and check if anything changed regarding > >> these macros (unlikely). > >> > >> > __STRICT_ANSI__ is defined if Clang has not been asked to provide > >> > extensions (either GNU extensions, perhaps via a flag like -std=gnu99, > >> > or MS extensions), and is used by C library headers to determine that > >> > they should provide a strictly-conforming set of declarations without > >> > extensions. > >> Ok, so if !defined(__STRICT__ANSI__) clang should always expose "as > >> much as possible", including stuff from later versions of the Std. > >> and/or eventual extensions, just as it now on float.h and float.c, > >> right? > >> > >> > Testing __STDC_VERSION__ for C94 makes sense if you're trying to > >> > detect whether Amendment 1 features should be provided. > >> Since this will affect only digraphs, I guess there is no need (for > >> float.h, float.c). > >> > >> >> 3) Lastly, can you expand (...) > >> > > >> > No, it does not mean that. > >> > > >> > For PPC64, long double is (sometimes) modeled as a pair of doubles. > >> > Under that model, the smallest normalized value for long double is > >> > actually larger than the smallest normalized value for double > >> > (remember that for a normalized value with exponent E, all numbers of > >> > the form 1.XXXXX * 2^E, with the right number of mantissa digits, are > >> > exactly representable, so increasing the number of mantissa bits > >> > without changing the number of exponent bits increases the magnitude > >> > of the smallest normalized positive number). > >> > > >> > The set of values of long double in this model *is* a superset of the > >> > set of values of double. > >> > > >> I see now, and removed the bogus tests. The patch should now test > >> cleanly unless something needs DECIMAL_DIG but did not set the > >> appropriate std. level, or defined __STRICT__ANSI__. > >> > >> Thanks for the learning experience, > >> > >> JT > >> > >> > >> > >> >> From /test/Preprocessor/init.cpp: > >> >> // PPC64:#define __DBL_MIN_EXP__ (-1021) > >> >> // PPC64:#define __FLT_MIN_EXP__ (-125) > >> >> // PPC64:#define __LDBL_MIN_EXP__ (-968) > >> >> > >> >> This issue happened before > >> >> (https://lists.gnu.org/archive/html/bug-gnulib/2011-08/msg00262.html > , > >> >> http://www.openwall.com/lists/musl/2013/11/15/1), but all it means > is > >> >> that ppc64 is not compliant with C without soft-float. The test is > >> >> valid and should stay, and if someone tries to compile for ppc64 in > >> >> c89, c99 or c11 modes, clang should 1) use soft float (bad idea), 2) > >> >> issue a diagnostic saying that that arch cannot meet the desired C > >> >> standard without a big performance penalty - the diag should be > >> >> suppressible with some special cmd line argument. > >> >> Thus, I added the tests back and the FAIL for PPC64 for the time > >> >> being, with a comment. If you know of a way to skip only the specific > >> >> *_MIN_EXP and *_MIN_10_EXP tests, please add it, because there might > >> >> be more similar cases in the future. > >> >> > >> >> JT > >> >> > >> >> On Thu, Feb 11, 2016 at 3:04 PM, Richard Smith < > rich...@metafoo.co.uk> > >> >> wrote: > >> >>> Thanks, I modified the test to also test C89 and C99 modes and > >> >>> committed this as r260577. > >> >>> > >> >>> On Thu, Feb 11, 2016 at 11:29 AM, Jorge Teixeira > >> >>> <j.lopes.teixe...@gmail.com> wrote: > >> >>>> Here is a revised test, which I renamed to c11-5_2_4_2_2p11.c > instead > >> >>>> of float.c because I am only checking a subset of what the standard > >> >>>> mandates for float.h, and because there were similar precedents, > like > >> >>>> test/Preprocessor/c99-*.c. Feel free to override, though. > >> >>> > >> >>> test/Preprocessor/c99-* are an aberration. The goal would be that > this > >> >>> test grows to cover all of the parts of float.h that we define, so > >> >>> float.c seems like the appropriate name for it. > >> >>> > >> >>>> The first part checks for basic compliance with the referred C11 > >> >>>> paragraph, the second for internal consistency between the > >> >>>> underscored > >> >>>> and exposed versions of the macros. > >> >>>> No attempt was made to support C99 or C89. > >> >>>> > >> >>>> I am not very clear on the proper use of the whole lit.py / RUN > >> >>>> framework, so someone should really confirm if what I wrote is > >> >>>> correct. The goal was to test both hosted and freestanding > >> >>>> implementations with C11, and expect no diagnostics from either. > >> >>> > >> >>> We generally avoid testing hosted mode, because we don't want the > >> >>> success of our tests to depend on the libc installed on the host > >> >>> system. > >> >>> > >> >>>> Thanks for the help, > >> >>>> > >> >>>> JT > >> >>>> > >> >>>> On Tue, Feb 9, 2016 at 5:56 PM, Richard Smith < > rich...@metafoo.co.uk> > >> >>>> wrote: > >> >>>>> On Tue, Feb 9, 2016 at 2:43 PM, Jorge Teixeira > >> >>>>> <j.lopes.teixe...@gmail.com> wrote: > >> >>>>>> Richard, > >> >>>>>> > >> >>>>>> Can you be more specific? > >> >>>>>> > >> >>>>>> I assume you mean something like my newly attached .h file that > >> >>>>>> tests > >> >>>>>> very basic implementation compliance (i.e., it's required, but > not > >> >>>>>> sufficient), but I would need a bit more guidance about the > >> >>>>>> structure > >> >>>>>> of the file, how to perform the tests, and where to exactly place > >> >>>>>> and > >> >>>>>> name the file within test/Headers. > >> >>>>>> > >> >>>>>> I some sort of template exists, or if someone else takes point > and > >> >>>>>> makes it, I can "port" the attached p11 test cases. I am unsure > of > >> >>>>>> how > >> >>>>>> to perform a more normative compliance - for example, to assert > >> >>>>>> that > >> >>>>>> LDBL_DECIMAL_DIG is 21 on x86-64 and that indeed those many > digits > >> >>>>>> are > >> >>>>>> guaranteed to be correct, etc. This is probably not possible / > does > >> >>>>>> not make sense. > >> >>>>> > >> >>>>> That looks like a decent basic test for this. The test should be > >> >>>>> named > >> >>>>> something like test/Headers/float.c, and needs to contain a "RUN:" > >> >>>>> line so that the test runner infrastructure knows how to run it. > You > >> >>>>> can look at test/Header/limits.cpp for an example of how this > works. > >> >>>>> > >> >>>>> We already have platform-specific tests that __LDBL_DECIMAL_DIG__ > is > >> >>>>> the right value, so you could test the values are correct by > >> >>>>> checking > >> >>>>> that LDBL_DECIMAL_DIG == __LDBL_DECIMAL_DIG__. > >> >>>>> > >> >>>>>> JT > >> >>>>>> > >> >>>>>> On Tue, Feb 9, 2016 at 3:58 PM, Richard Smith > >> >>>>>> <rich...@metafoo.co.uk> wrote: > >> >>>>>>> Patch looks good. Please also add a testcase to test/Headers. > >> >>>>>>> > >> >>>>>>> On Tue, Feb 9, 2016 at 12:08 PM, Hubert Tong via cfe-commits > >> >>>>>>> <cfe-commits@lists.llvm.org> wrote: > >> >>>>>>>> I see no immediate issue with this patch, but I am not one of > the > >> >>>>>>>> usual > >> >>>>>>>> reviewers for this part of the code base. > >> >>>>>>>> > >> >>>>>>>> -- HT > >> >>>>>>>> > >> >>>>>>>> > >> >>>>>>>> On Tue, Feb 9, 2016 at 2:56 PM, Jorge Teixeira > >> >>>>>>>> <j.lopes.teixe...@gmail.com> > >> >>>>>>>> wrote: > >> >>>>>>>>> > >> >>>>>>>>> Thanks Hubert. Somehow I omitted that prefix when typing the > >> >>>>>>>>> macros, > >> >>>>>>>>> and I did not noticed it when I was testing because on my arch > >> >>>>>>>>> DECIMAL_DIG is defined to be the LDBL version... > >> >>>>>>>>> > >> >>>>>>>>> Updated patch is attached. > >> >>>>>>>>> > >> >>>>>>>>> JT > >> >>>>>>>>> > >> >>>>>>>>> On Tue, Feb 9, 2016 at 1:41 PM, Hubert Tong > >> >>>>>>>>> <hubert.reinterpretc...@gmail.com> wrote: > >> >>>>>>>>> > There is a __LDBL_DECIMAL_DIG__ predefined macro. > >> >>>>>>>>> > __DECIMAL_DIG__ will > >> >>>>>>>>> > not > >> >>>>>>>>> > always be the same as __LDBL_DECIMAL_DIG__. > >> >>>>>>>>> > > >> >>>>>>>>> > -- HT > >> >>>>>>>>> > > >> >>>>>>>>> > On Mon, Feb 8, 2016 at 11:26 PM, Jorge Teixeira via > >> >>>>>>>>> > cfe-commits > >> >>>>>>>>> > <cfe-commits@lists.llvm.org> wrote: > >> >>>>>>>>> >> > >> >>>>>>>>> >> Hi, I filed the bug > >> >>>>>>>>> >> (https://llvm.org/bugs/show_bug.cgi?id=26283) some > >> >>>>>>>>> >> time ago and nobody picked it up, so here is a trivial > patch > >> >>>>>>>>> >> exposing > >> >>>>>>>>> >> the missing macros, that to the best of my ability were > >> >>>>>>>>> >> already > >> >>>>>>>>> >> present as the internal underscored versions. > >> >>>>>>>>> >> > >> >>>>>>>>> >> Perhaps a more general bug about C11 floating point (lack > of) > >> >>>>>>>>> >> conformance should be filed, so that some form of unit > >> >>>>>>>>> >> test/macro > >> >>>>>>>>> >> validation could be worked on, but this patch does scratch > my > >> >>>>>>>>> >> current > >> >>>>>>>>> >> itch. > >> >>>>>>>>> >> > >> >>>>>>>>> >> Successfully tested on x86-64 Xubuntu 14.04 with clang 3.8 > >> >>>>>>>>> >> from the > >> >>>>>>>>> >> ppa, patched with the attached diff. > >> >>>>>>>>> >> > >> >>>>>>>>> >> First contribution, so feel free to suggest improvements or > >> >>>>>>>>> >> point to > >> >>>>>>>>> >> more detailed step-by-step instructions/guidelines. > >> >>>>>>>>> >> > >> >>>>>>>>> >> Cheers, > >> >>>>>>>>> >> > >> >>>>>>>>> >> JT > >> >>>>>>>>> >> > >> >>>>>>>>> >> _______________________________________________ > >> >>>>>>>>> >> cfe-commits mailing list > >> >>>>>>>>> >> cfe-commits@lists.llvm.org > >> >>>>>>>>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > >> >>>>>>>>> >> > >> >>>>>>>>> > > >> >>>>>>>> > >> >>>>>>>> > >> >>>>>>>> > >> >>>>>>>> _______________________________________________ > >> >>>>>>>> cfe-commits mailing list > >> >>>>>>>> cfe-commits@lists.llvm.org > >> >>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > >> >>>>>>>> > > > > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits