This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [include] Declare __STDC_*_MACROS for C++11 compat in old libc
ClosedPublic

Authored by mgorny on Sep 25 2016, 1:42 PM.

Details

Summary

Declare STDC_FORMAT_MACROS, STDC_LIMIT_MACROS and __STDC_CONSTANT_MACROS before including real inttypes.h/stdint.h when the wrapper-header is included in C++11, in order to enable the necessary macros in C99-compliant libc.

The C99 standard defined that the format macros in inttypes.h should be defined by the C++ implementations only when STDC_FORMAT_MACROS is defined, and the limit and constant macros in stdint.h should be defined only when STDC_LIMIT_MACROS and __STDC_CONSTANT_MACROS are defined appropriately. Following this specification, multiple old versions of glibc up to 2.17 do not define those macros by default for C++, rendering the libc++ headers non-compliant to the C++11 standard.

In order to achieve the necessary compliance, STDC_FORMAT_MACROS is defined in wrapped inttypes.h just before including the system inttypes.h, when C++11 or newer is used. Both STDC_LIMIT_MACROS and __STDC_CONSTANT_MACROS are defined in newly-wrapped stdint.h. This fixes the C++11 compliance while preserving the current behavior for C++03.

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny updated this revision to Diff 72434.Sep 25 2016, 1:42 PM
mgorny retitled this revision from to [libcxx] [include] Declare __STDC_*_MACROS for C++11 compat in old libc.
mgorny updated this object.
mgorny added a reviewer: EricWF.
mgorny added a subscriber: cfe-commits.
EricWF added subscribers: mclow.lists, rsmith.

Adding reviewers @mclow.lists and @rsmith since he originally wrote the C wrappers.

When the <cfoo> headers where split into <cfoo> and <foo.h> headers in D12747, we have left out <cstdint> and <ctime>. I was going to put up a patch to fix this but couldn't get to it. It's good that you are adding <stdint.h>, perhaps you can follow what was done in D12747 and repeat the same for <cstdint>?

You will also need a test for the new header, D12747 has examples.

Hope this helps!

/ Asiri

EricWF added inline comments.Sep 25 2016, 2:18 PM
include/stdint.h
24 ↗(On Diff #72434)

Please take a look at how clang/lib/Headers/stdint.h handles forwarding this header, and use a solution similar to that.

27 ↗(On Diff #72434)

I think we should define __STDC_LIMIT_MACROS and __STDC_CONSTANT_MACROS even if we are in C++03, since we don't provide strict C++03 conformance. However we should still check if __cplusplus is defined.

When the <cfoo> headers where split into <cfoo> and <foo.h> headers in D12747, we have left out <cstdint> and <ctime>. I was going to put up a patch to fix this but couldn't get to it. It's good that you are adding <stdint.h>, perhaps you can follow what was done in D12747 and repeat the same for <cstdint>?

I'm sorry but could you be a little more specific, please? I've looked into that diff, and it seems that the cstdint resembles e.g. cinttypes already -- i.e. there's only the include, and 'using' lines there. Or do you mean that I should copy/update the synopsis over to stdint.h?

mgorny updated this revision to Diff 72441.Sep 26 2016, 12:04 AM
mgorny edited edge metadata.
mgorny marked an inline comment as done.Sep 26 2016, 12:07 AM

When the <cfoo> headers where split into <cfoo> and <foo.h> headers in D12747, we have left out <cstdint> and <ctime>. I was going to put up a patch to fix this but couldn't get to it. It's good that you are adding <stdint.h>, perhaps you can follow what was done in D12747 and repeat the same for <cstdint>?

You will also need a test for the new header, D12747 has examples.

I see that libcxx already has a pretty complete test for stdint.h.

After consulting the standard, I've shamelessly copied the C part of synopsis to my stdint.h.

mgorny added inline comments.Sep 26 2016, 12:10 AM
include/stdint.h
25 ↗(On Diff #72441)

Are you referring to the way __STDC_LIMIT_MACROS etc. is handled there? Or something more?

I'm sorry but could you be a little more specific, please? I've looked into that diff, and it seems that the cstdint resembles e.g. cinttypes already -- i.e. there's only the include, and 'using' lines there. Or do you mean that I should copy/update the synopsis over to stdint.h?

My bad, I hadn't actually looked at the current status of <cstdint>, it doesn't seem to need any more work.

/ Asiri

I see that libcxx already has a pretty complete test for stdint.h.

Hmm, now I see that too :)

Thanks for the patch.

/ Asiri

mgorny updated this revision to Diff 72492.Sep 26 2016, 8:35 AM

@EricWF, I've copied the additional unset logic from clang. Does this address all of your concerns?

By the way, I think clang misses __STDC_FORMAT_MACROS for inttypes.h. I'll write a patch for that as well, for consistency.

EricWF accepted this revision.Sep 26 2016, 1:20 PM
EricWF edited edge metadata.

LGTM. Please remove the LIBCXX macros before committing.

include/stdint.h
118 ↗(On Diff #72492)

We don't actually need the __STDC_CONSTANT_MACROS_DEFINED_BY_LIBCXX. Please remove before commiting.

This revision is now accepted and ready to land.Sep 26 2016, 1:20 PM

Ok. Thanks for the review.

This revision was automatically updated to reflect the committed changes.