Committed as r260710 <http://reviews.llvm.org/rL260710>.
On Fri, Feb 12, 2016 at 9:53 AM, Hubert Tong < hubert.reinterpretc...@gmail.com> wrote: > 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
