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