Skip to content

Check sizeof long using the more portable LONG_BIT.#2214

Closed
jimis wants to merge 1 commit into
curl:masterfrom
jimis:master
Closed

Check sizeof long using the more portable LONG_BIT.#2214
jimis wants to merge 1 commit into
curl:masterfrom
jimis:master

Conversation

@jimis
Copy link
Copy Markdown
Contributor

@jimis jimis commented Jan 3, 2018

The previous attempt (reverted with commit
272613d ) used macro SIZEOF_LONG which was
not a system include, but internal to cURL, defined from configure.ac
AC_CHECK_SIZEOF(long). Using LONG_BIT is more portable.

Even LONG_BIT though requires pulling in <limits.h> and _XOPEN_SOURCE to be
defined. We include <limits.h> only inside the specific ifdef in order to
avoid affecting other platforms that already work.

The previous attempt (reverted with commit
272613d ) used macro SIZEOF_LONG which was
not a system include, but internal to cURL, defined from configure.ac
AC_CHECK_SIZEOF(long). Using LONG_BIT is more portable.

Even LONG_BIT though requires pulling in <limits.h> and _XOPEN_SOURCE to be
defined. We include <limits.h> only inside the specific ifdef in order to
avoid affecting other platforms that already work.
@jimis
Copy link
Copy Markdown
Contributor Author

jimis commented Jan 3, 2018

This is another attempt after #2186 was reverted, and seems to work on all our buildslaves, including the previously broken AIX and HP-UX.

@jay
Copy link
Copy Markdown
Member

jay commented Jan 3, 2018

limits.h is guarded in curl source by HAVE_LIMITS_H so it apparently isn't always available. reviewing c89 I see it should always be available even in the most strict implementations, so i put in #2215 to address that. however even if that is approved i don't think we should define _XOPEN_SOURCE in curl/system.h. note your commit is currently missing xopen define. i think it would be better as defined(__LONG_MAX__) && __LONG_MAX__ == 2147483647L. also i suggest doing __LONG_LONG the same way for the 64 bit section

@jay
Copy link
Copy Markdown
Member

jay commented Jan 3, 2018

oops I meant 64-bit __LONG_MAX not LONG LONG to check if long is 64-bits.

@jimis
Copy link
Copy Markdown
Contributor Author

jimis commented Jan 4, 2018

Thanks. Indeed I forgot the guard for including limits.h, I can add it if #2215 is not to be merged.

On the other hand I did not define _XOPEN_SOURCE on purpose, as that would be very intrusive. Nevertheless LONG_BIT was defined on the not-so-recent UNIX hosts I tested it on (AIX 5.3, HP-UX 11.23).

IMO the clear way would be to include <limits.h> at the top and test for LONG_BIT and/or LONG_MAX (the latter is c89) at the beginning, so then there is no need to go into compiler dependant hacks. Avoided in the end because of potential breakage in obscure non standard platforms. Regarding other solutions:

  • __LONG_MAX__: as I mentioned in Fix compilation with gcc on AIX PPC and IA64 HP-UX. #2186 it was introduced in gcc 3.3 (I don't know if you want to support older compilers), plus I've heard of platforms that reserve bits, so the MAX and MIN values are smaller.
  • Checking for the defines _POWER or __ia64 will also work for my platforms. Strangely the already tested ones __ppc__ and __powerpc__ are not defined on the ppc AIX box. I'm sure this has to do with GCC being old (version 4.2).

i think it would be better as defined(LONG_MAX) && LONG_MAX == 2147483647L. also i suggest doing __LONG_LONG the same way for the 64 bit section

I'll submit a new pull request for this then, feel free to close this one.

@bagder
Copy link
Copy Markdown
Member

bagder commented Jan 9, 2018

See #2216

@bagder bagder closed this Jan 9, 2018
@lock lock Bot locked as resolved and limited conversation to collaborators May 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants