This is an archive of the discontinued LLVM Phabricator instance.

_Float16 preprocessor macro definitions
ClosedPublic

Authored by SjoerdMeijer on Jun 27 2017, 8:45 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

SjoerdMeijer created this revision.Jun 27 2017, 8:45 AM
rogfer01 added inline comments.Sep 12 2017, 8:32 AM
lib/Headers/float.h
137 ↗(On Diff #104183)

My understanding is that, given that we support TS18661-2 by default, this macro should be predefined by clang and then there is no need to protect these macros.

You may want to add a test for this in test/Preprocessor/init.c.

139–142 ↗(On Diff #104183)

There is a typo here, these macros should start with __FLT16_. Add a test for these names test/Headers/float.c.

scanon added a subscriber: scanon.Sep 12 2017, 8:36 AM
scanon added inline comments.
lib/Headers/float.h
137 ↗(On Diff #104183)

Where do you see that the __STDC_WANT_IEC_60559_TYPES_EXT__ macro should be predefined by clang?

rogfer01 added inline comments.Sep 12 2017, 8:56 AM
lib/Headers/float.h
137 ↗(On Diff #104183)

Hi Steve,

certainly you're right, the TS says

The new identifiers added to C11 library headers by this part of ISO/IEC TS-18661 are defined or declared by their respective headers only if __STDC_WANT_IEC_60559_TYPES_EXT__ is defined as a macro at the point in the source file where the appropriate header is first included.

so (if I read this right) these identifiers are only available if such macro is defined when including float.h.

Can I assume from your comment that someone else should define it? Perhaps the float.h header itself, some other file in the C-library implementation or the user of the compiler via some -D__STDC_WANT_IEC_60559_TYPES_EXT__, but not be predefined by the compiler? If this is the case, then the macros still have to be guarded conditionally (as they were in the original patch).

Does this make sense? Thanks.

scanon added inline comments.Sep 12 2017, 8:59 AM
lib/Headers/float.h
137 ↗(On Diff #104183)

I think we could justify defining it ourselves under non-strict compilation modes; alternatively, system headers might define it for users in non-strict modes.

My reading of the TS is that in strict mode, these types and macros should be hidden unless the user explicitly requests them by defining __STDC_WANT_IEC_60559_TYPES_EXT__ themselves.

SjoerdMeijer added inline comments.Sep 12 2017, 9:05 AM
lib/Headers/float.h
137 ↗(On Diff #104183)

Thanks, very useful discussion and clarification. I will add some tests for this, which I indeed forgot. Cheers.

Fixed the typos, and added tests.

rogfer01 accepted this revision.Sep 13 2017, 7:57 AM
rogfer01 added a subscriber: rsmith.

This LGTM, but wait a couple of days before comitting in case @rsmith or @scanon (or others!) have further comments.

This revision is now accepted and ready to land.Sep 13 2017, 7:57 AM
scanon accepted this revision.Sep 13 2017, 8:17 AM

LGTM as well.

many thanks for reviewing and your help.

This revision was automatically updated to reflect the committed changes.