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
>> >>>>>>>>
>
>
Index: test/Headers/float.c
===================================================================
--- test/Headers/float.c (revision 260651)
+++ test/Headers/float.c (working copy)
@@ -24,17 +24,17 @@
#ifndef FLT_MANT_DIG
#error "Mandatory macro FLT_MANT_DIG is missing."
-#elif FLT_MANT_DIG < 1
+#elif FLT_MANT_DIG < 2
#error "Mandatory macro FLT_MANT_DIG is invalid."
#endif
#ifndef DBL_MANT_DIG
#error "Mandatory macro DBL_MANT_DIG is missing."
-#elif DBL_MANT_DIG < 1
+#elif DBL_MANT_DIG < 2
#error "Mandatory macro DBL_MANT_DIG is invalid."
#endif
#ifndef LDBL_MANT_DIG
#error "Mandatory macro LDBL_MANT_DIG is missing."
-#elif LDBL_MANT_DIG < 1
+#elif LDBL_MANT_DIG < 2
#error "Mandatory macro LDBL_MANT_DIG is invalid."
#endif
#if ((FLT_MANT_DIG > DBL_MANT_DIG) || (DBL_MANT_DIG > LDBL_MANT_DIG))
@@ -108,18 +108,18 @@
#ifndef FLT_MIN_EXP
- #error "Mandatory macro _MIN_EXP is missing."
-#elif FLT_MIN_EXP > -2
- #error "Mandatory macro _MIN_EXP is invalid."
+ #error "Mandatory macro FLT_MIN_EXP is missing."
+#elif FLT_MIN_EXP > -1
+ #error "Mandatory macro FLT_MIN_EXP is invalid."
#endif
#ifndef DBL_MIN_EXP
#error "Mandatory macro DBL_MIN_EXP is missing."
-#elif DBL_MIN_EXP > -2
+#elif DBL_MIN_EXP > -1
#error "Mandatory macro DBL_MIN_EXP is invalid."
#endif
#ifndef LDBL_MIN_EXP
#error "Mandatory macro LDBL_MIN_EXP is missing."
-#elif LDBL_MIN_EXP > -2
+#elif LDBL_MIN_EXP > -1
#error "Mandatory macro LDBL_MIN_EXP is invalid."
#endif
@@ -143,17 +143,17 @@
#ifndef FLT_MAX_EXP
#error "Mandatory macro FLT_MAX_EXP is missing."
-#elif FLT_MAX_EXP < 2
+#elif FLT_MAX_EXP < 1
#error "Mandatory macro FLT_MAX_EXP is invalid."
#endif
#ifndef DBL_MAX_EXP
#error "Mandatory macro DBL_MAX_EXP is missing."
-#elif DBL_MAX_EXP < 2
+#elif DBL_MAX_EXP < 1
#error "Mandatory macro DBL_MAX_EXP is invalid."
#endif
#ifndef LDBL_MAX_EXP
#error "Mandatory macro LDBL_MAX_EXP is missing."
-#elif LDBL_MAX_EXP < 2
+#elif LDBL_MAX_EXP < 1
#error "Mandatory macro LDBL_MAX_EXP is invalid."
#endif
#if ((FLT_MAX_EXP > DBL_MAX_EXP) || (DBL_MAX_EXP > LDBL_MAX_EXP))
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
