This is an archive of the discontinued LLVM Phabricator instance.

Do not require __STDC_LIMIT_MACROS and others
ClosedPublic

Authored by chfast on Jun 21 2016, 5:08 AM.

Details

Summary

Do not require STDC_LIMIT_MACROS and STDC_CONSTANT_MACROS macros to be defined globally. They are not needed for C++11 compliant standard headers.

Diff Detail

Repository
rL LLVM

Event Timeline

chfast updated this revision to Diff 61357.Jun 21 2016, 5:08 AM
chfast retitled this revision from to Do not require __STDC_LIMIT_MACROS and others.
chfast updated this object.
chfast added a subscriber: llvm-commits.

Is there a way to test this commit on buildbot before merging it?

joerg edited edge metadata.Jun 21 2016, 8:22 AM

No longer forcing them is fine, but no longer defining them is IMO asking for bootstrap problems without a good reason.

jyknight edited edge metadata.Jun 21 2016, 8:24 AM

Not requiring the defines is good; removing the defines in the build files is not. We still want to be able to build out of the box on systems with this known brokenness.

cmake/modules/HandleLLVMOptions.cmake
555–557 ↗(On Diff #61357)

Keep these.

utils/vim/vimrc
111 ↗(On Diff #61357)

Probably leave this too.

Not requiring the defines is good; removing the defines in the build files is not. We still want to be able to build out of the box on systems with this known brokenness.

Can we identify such broken systems in the first place?

I do care about exported cmake variables, especially LLVM_DEFINITIONS and it currently looks like:

set(LLVM_DEFINITIONS "-D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS")

I guess nobody knows any more why and where they are needed.

chfast updated this revision to Diff 61529.Jun 22 2016, 4:06 AM
chfast edited edge metadata.

Leave defines config as it was before.

Not requiring the defines is good; removing the defines in the build files is not. We still want to be able to build out of the box on systems with this known brokenness.

Can we identify such broken systems in the first place?

Looking around briefly at the libcs I have handy...
glibc stopped checking them in 2013, newlib dropped them in 2015, musl in 2013, bionic in 2014. (dunno release dates, those are commit dates). So things look pretty good there.

On the other hand, you have platforms like freebsd and netbsd, which apparently *still* haven't removed these #ifdefs from their stdint.h. So...yeah, seems like there's still a need for the defines, for now. Anyone over in BSD land paying attention?... /me pokes joerg. :)

Anyway, I think is it good point to at least separate the changes as you suggested.

jyknight accepted this revision.Jun 22 2016, 11:44 AM
jyknight edited edge metadata.

Oops, forgot to say LGTM last time. I said it in my head. :)

This revision is now accepted and ready to land.Jun 22 2016, 11:44 AM
This revision was automatically updated to reflect the committed changes.