Page MenuHomePhabricator

[clang] Make sure C99/C11 features in <float.h> are provided in C++11
ClosedPublic

Authored by ldionne on Feb 12 2019, 2:25 PM.

Diff Detail

Repository
rC Clang

Event Timeline

ldionne created this revision.Feb 12 2019, 2:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2019, 2:25 PM
jfb accepted this revision.Feb 12 2019, 2:43 PM
This revision is now accepted and ready to land.Feb 12 2019, 2:43 PM

I'll ship this. @eli.friedman I think this is your playground -- if you want any changes to happen post-review, please LMK and I will gladly cooperate.

This revision was automatically updated to reflect the committed changes.

Formally, I don't think C11 is a normative reference for C++11 or C++14, only C++17 (see [intro.refs] in the standard).

jfb added a comment.EditedFeb 13 2019, 3:58 PM

Formally, I don't think C11 is a normative reference for C++11 or C++14, only C++17 (see [intro.refs] in the standard).

Right, this was changed in wg21.link/p0063r3
That being said, we *can* offer these C11 features as extensions IIUC.

In D58149#1397390, @jfb wrote:

Formally, I don't think C11 is a normative reference for C++11 or C++14, only C++17 (see [intro.refs] in the standard).

You're right. I don't mind submitting a patch that enables it for C++17 and above only if that's what you want, however...

Right, this was changed in wg21.link/p0063r3
That being said, we *can* offer these C11 features as extensions IIUC.

... I think it's also fine to have it in C++11.

Your choice, @eli.friedman

rsmith added a subscriber: rsmith.Feb 13 2019, 6:05 PM
In D58149#1397390, @jfb wrote:

Formally, I don't think C11 is a normative reference for C++11 or C++14, only C++17 (see [intro.refs] in the standard).

You're right. I don't mind submitting a patch that enables it for C++17 and above only if that's what you want, however...

Right, this was changed in wg21.link/p0063r3
That being said, we *can* offer these C11 features as extensions IIUC.

... I think it's also fine to have it in C++11.

In C++11, this is technically valid:

#define FLT_TRUE_MIN 1
#include <cfloat>
#if FLT_TRUE_MIN != 1
#error float.h redefined my macro
#endif

and likewise

#include <cfloat>
constexpr float FLT_TRUE_MIN = 0.0001f;

... but both will fail with this patch, so providing these macros is technically non-conforming. I don't know to what extent we should care about such uses, though. We generally leak additional non-conforming names from the system <*.h> headers into the user's namespace if the libc headers put them there. This would, however, probably be the first time that we put them there ourselves.

jfb added a comment.Feb 13 2019, 9:10 PM
In D58149#1397390, @jfb wrote:

Formally, I don't think C11 is a normative reference for C++11 or C++14, only C++17 (see [intro.refs] in the standard).

You're right. I don't mind submitting a patch that enables it for C++17 and above only if that's what you want, however...

Right, this was changed in wg21.link/p0063r3
That being said, we *can* offer these C11 features as extensions IIUC.

... I think it's also fine to have it in C++11.

In C++11, this is technically valid:

#define FLT_TRUE_MIN 1
#include <cfloat>
#if FLT_TRUE_MIN != 1
#error float.h redefined my macro
#endif

and likewise

#include <cfloat>
constexpr float FLT_TRUE_MIN = 0.0001f;

... but both will fail with this patch, so providing these macros is technically non-conforming. I don't know to what extent we should care about such uses, though. We generally leak additional non-conforming names from the system <*.h> headers into the user's namespace if the libc headers put them there. This would, however, probably be the first time that we put them there ourselves.

🎻 ← tiny unicode violin for anyone this would break ☺️

I agree with the comments. I think we should strive to be strictly conforming. I make that argument all the time for libc++, so I'll be consistent :-).

https://reviews.llvm.org/D58289