starred/brotli
1
0
Fork 0
mirror of https://github.com/google/brotli.git synced 2026-07-08 17:56:58 +00:00
Code Issues 1.2k Packages Projects Releases 20 Wiki Activity

Issues with portability code #224

New Issue
Closed
opened 2026-01-29 20:40:13 +00:00 by claunia · 11 comments
claunia commented 2026-01-29 20:40:13 +00:00
Owner
Copy Link

Originally created by @nemequ on GitHub (Apr 16, 2018).

I was just looking at one of the open PRs, which led me to c/include/brotli/port.h, and I noticed some issues.

Feature test macros defined in public headers outside of Brotli's namespace

This is the only real bug IMHO; the other stuff in this issue are really enhancements.

Brotli defines feature test macros (__has_*) in the public headers. In Brotli, you just use them like (__GNUC__ >= x) || __has_attribute(foo), but IMHO a better way to use them is something like

#if defined(__has_attribute)
#  if __has_attribute(foo)
__attribute__((__foo__))
#  endif
#elif defined(__GNUC__) && (__GNUC__ >= x)
__attribute__((__foo__))
#endif

I hide this behind a macro so it's easier to use, of course.

The basic idea is that if a feature test macro is defined you should trust that, and only check the version number as a fallback. This is important for compilers which define GNUC but don't support all the features from the version they define it to (which is annoyingly common).

If you don't want to do it that way in Brotli that's fine, but defining __has_* in your public header messes things up for people who are just using Brotli.

The most straightforward "fix" would be to just define BROTLI_HAS_* (or whatever you want to call it) instead of __has_* and use that. Another option would be to do something like I do in Hedley (or you can just use Hedley if you want; it's public domain / CC0):

#if defined(__has_attribute)
#  define BROTLI_GNUC_HAS_ATTRIBUTE(attribute,major,minor) __has_attribute(attribute)
#else
#  define BROTLI_GNUC_HAS_ATTRIBUTE(attribute,major,minor) (BROTLI_GCC_VERSION >= ((major * 100) + minor))
#endif

BROTLI_MODERN_COMPILER makes for an all-or-nothing approach

Defining BROTLI_MODERN_COMPILER and using it everywhere to check for features means that if a compiler doesn't implement everything Brotli can take advantage of then Brotli can't take advantage of anything.

For example, BROTLI_DEPRECATED is only defined for GCC ≥ 3.4, but the __attribute__((__deprecated__)) is available in GCC ≥ 3.1.

This will prefer __has_attribute, but still provides an easy way to test the GNUC version.

Many macros could be implemented for more compilers

For example, BROTLI_DEPRECATED. Here is the macro from Hedley, which covers GCC, clang, ICC, ARM, Sun CC, PGI, TI, IAR, and MSVC.

#if defined(__cplusplus) && (__cplusplus >= 201402L)
#  define HEDLEY_DEPRECATED(since) [[deprecated("Since " #since)]]
#  define HEDLEY_DEPRECATED_FOR(since, replacement) [[deprecated("Since " #since "; use " #replacement)]]
#elif \
  HEDLEY_GNUC_HAS_EXTENSION(attribute_deprecated_with_message,4,5,0) || \
  HEDLEY_INTEL_VERSION_CHECK(16,0,0) || \
  HEDLEY_ARM_VERSION_CHECK(5,6,0) || \
  HEDLEY_SUNPRO_VERSION_CHECK(5,13,0) || \
  HEDLEY_PGI_VERSION_CHECK(17,10,0)
#  define HEDLEY_DEPRECATED(since) __attribute__((__deprecated__("Since " #since)))
#  define HEDLEY_DEPRECATED_FOR(since, replacement) __attribute__((__deprecated__("Since " #since "; use " #replacement)))
#elif \
  HEDLEY_GNUC_HAS_ATTRIBUTE(deprcated,3,1,0) || \
  HEDLEY_ARM_VERSION_CHECK(4,1,0) || \
  HEDLEY_TI_VERSION_CHECK(8,0,0) || \
  (HEDLEY_TI_VERSION_CHECK(7,3,0) && defined(__TI_GNU_ATTRIBUTE_SUPPORT__))
#  define HEDLEY_DEPRECATED(since) __attribute__((__deprecated__))
#  define HEDLEY_DEPRECATED_FOR(since, replacement) __attribute__((__deprecated__))
#elif HEDLEY_MSVC_VERSION_CHECK(14,0,0)
#  define HEDLEY_DEPRECATED(since) __declspec(deprecated("Since " # since))
#  define HEDLEY_DEPRECATED_FOR(since, replacement) __declspec(deprecated("Since " #since "; use " #replacement))
#elif HEDLEY_MSVC_VERSION_CHECK(13,10,0)
#  define HEDLEY_DEPRECATED(since) _declspec(deprecated)
#  define HEDLEY_DEPRECATED_FOR(since, replacement) __declspec(deprecated)
#elif HEDLEY_IAR_VERSION_CHECK(8,0,0)
#  define HEDLEY_DEPRECATED(since) _Pragma("deprecated")
#  define HEDLEY_DEPRECATED_FOR(since, replacement) _Pragma("deprecated")
#else
#  define HEDLEY_DEPRECATED(since)
#  define HEDLEY_DEPRECATED_FOR(since, replacement)
#endif

I see there are some more macros in c/platform.h which are similarly limited, such as BROTLI_ATTRIBUTE_VISIBILITY_HIDDEN, BROTLI_INLINE, BROTLI_RESTRICT, BROTLI_NOINLINE, BROTLI_PREDICT_TRUE, BROTLI_PREDICT_FALSE. They all have corresponding macros in Hedley already (HEDLEY_PRIVATE, HEDLEY_INLINE, HEDLEY_RESTRICT, HEDLEY_NEVER_INLNIE, HEDLEY_LIKELY, HEDLEY_UNLIKELY) so you can just copy the version numbers from there (or use Hedley).

BROTLI_IS_CONSTANT could also be implemented for other compilers; several do implement __builtin_constant_p, but I'd need to do some research to figure out which ones. For compilers where sizeof(void) works you can also use this monstrosity, or this version for C11+.

Originally created by @nemequ on GitHub (Apr 16, 2018). I was just looking at one of the open PRs, which led me to `c/include/brotli/port.h`, and I noticed some issues. ### Feature test macros defined in public headers outside of Brotli's namespace This is the only real bug IMHO; the other stuff in this issue are really enhancements. Brotli defines feature test macros (`__has_*`) in the public headers. In Brotli, you just use them like `(__GNUC__ >= x) || __has_attribute(foo)`, but IMHO a better way to use them is something like ```c #if defined(__has_attribute) # if __has_attribute(foo) __attribute__((__foo__)) # endif #elif defined(__GNUC__) && (__GNUC__ >= x) __attribute__((__foo__)) #endif ``` I hide this behind a macro so it's easier to use, of course. The basic idea is that if a feature test macro is defined you should trust that, and only check the version number as a fallback. This is important for compilers which define __GNUC__ but don't support all the features from the version they define it to (which is annoyingly common). If you don't want to do it that way in Brotli that's fine, but defining `__has_*` in your public header messes things up for people who are just using Brotli. The most straightforward "fix" would be to just define `BROTLI_HAS_*` (or whatever you want to call it) instead of `__has_*` and use that. Another option would be to do something [like I do](https://github.com/nemequ/hedley/blob/18dd7a5420340cdd23310b6a6790823a635ded21/hedley.h#L325) in [Hedley](https://nemequ.github.io/hedley/) (or you can just use Hedley if you want; it's public domain / CC0): ```c #if defined(__has_attribute) # define BROTLI_GNUC_HAS_ATTRIBUTE(attribute,major,minor) __has_attribute(attribute) #else # define BROTLI_GNUC_HAS_ATTRIBUTE(attribute,major,minor) (BROTLI_GCC_VERSION >= ((major * 100) + minor)) #endif ``` ### `BROTLI_MODERN_COMPILER` makes for an all-or-nothing approach Defining `BROTLI_MODERN_COMPILER` and using it everywhere to check for features means that if a compiler doesn't implement everything Brotli can take advantage of then Brotli can't take advantage of anything. For example, `BROTLI_DEPRECATED` is only defined for GCC ≥ 3.4, but the `__attribute__((__deprecated__))` is available in GCC ≥ 3.1. This will prefer `__has_attribute`, but still provides an easy way to test the GNUC version. ### Many macros could be implemented for more compilers For example, `BROTLI_DEPRECATED`. Here is the macro from Hedley, which covers GCC, clang, ICC, ARM, Sun CC, PGI, TI, IAR, and MSVC. ```c #if defined(__cplusplus) && (__cplusplus >= 201402L) # define HEDLEY_DEPRECATED(since) [[deprecated("Since " #since)]] # define HEDLEY_DEPRECATED_FOR(since, replacement) [[deprecated("Since " #since "; use " #replacement)]] #elif \ HEDLEY_GNUC_HAS_EXTENSION(attribute_deprecated_with_message,4,5,0) || \ HEDLEY_INTEL_VERSION_CHECK(16,0,0) || \ HEDLEY_ARM_VERSION_CHECK(5,6,0) || \ HEDLEY_SUNPRO_VERSION_CHECK(5,13,0) || \ HEDLEY_PGI_VERSION_CHECK(17,10,0) # define HEDLEY_DEPRECATED(since) __attribute__((__deprecated__("Since " #since))) # define HEDLEY_DEPRECATED_FOR(since, replacement) __attribute__((__deprecated__("Since " #since "; use " #replacement))) #elif \ HEDLEY_GNUC_HAS_ATTRIBUTE(deprcated,3,1,0) || \ HEDLEY_ARM_VERSION_CHECK(4,1,0) || \ HEDLEY_TI_VERSION_CHECK(8,0,0) || \ (HEDLEY_TI_VERSION_CHECK(7,3,0) && defined(__TI_GNU_ATTRIBUTE_SUPPORT__)) # define HEDLEY_DEPRECATED(since) __attribute__((__deprecated__)) # define HEDLEY_DEPRECATED_FOR(since, replacement) __attribute__((__deprecated__)) #elif HEDLEY_MSVC_VERSION_CHECK(14,0,0) # define HEDLEY_DEPRECATED(since) __declspec(deprecated("Since " # since)) # define HEDLEY_DEPRECATED_FOR(since, replacement) __declspec(deprecated("Since " #since "; use " #replacement)) #elif HEDLEY_MSVC_VERSION_CHECK(13,10,0) # define HEDLEY_DEPRECATED(since) _declspec(deprecated) # define HEDLEY_DEPRECATED_FOR(since, replacement) __declspec(deprecated) #elif HEDLEY_IAR_VERSION_CHECK(8,0,0) # define HEDLEY_DEPRECATED(since) _Pragma("deprecated") # define HEDLEY_DEPRECATED_FOR(since, replacement) _Pragma("deprecated") #else # define HEDLEY_DEPRECATED(since) # define HEDLEY_DEPRECATED_FOR(since, replacement) #endif ``` I see there are some more macros in `c/platform.h` which are similarly limited, such as `BROTLI_ATTRIBUTE_VISIBILITY_HIDDEN`, `BROTLI_INLINE`, `BROTLI_RESTRICT`, `BROTLI_NOINLINE`, `BROTLI_PREDICT_TRUE`, `BROTLI_PREDICT_FALSE`. They all have corresponding macros in Hedley already ([`HEDLEY_PRIVATE`](https://nemequ.github.io/hedley/api-reference.html#HEDLEY_PRIVATE), [`HEDLEY_INLINE`](https://nemequ.github.io/hedley/api-reference.html#HEDLEY_INLINE), [`HEDLEY_RESTRICT`](https://nemequ.github.io/hedley/api-reference.html#HEDLEY_RESTRICT), [`HEDLEY_NEVER_INLNIE`](https://nemequ.github.io/hedley/api-reference.html#HEDLEY_NEVER_INLINE), [`HEDLEY_LIKELY`](https://nemequ.github.io/hedley/api-reference.html#HEDLEY_LIKELY), [`HEDLEY_UNLIKELY`](https://nemequ.github.io/hedley/api-reference.html#HEDLEY_UNLIKELY)) so you can just copy the version numbers from there (or use Hedley). `BROTLI_IS_CONSTANT` could also be implemented for other compilers; several do implement `__builtin_constant_p`, but I'd need to do some research to figure out which ones. For compilers where `sizeof(void)` works you can also use [this monstrosity](https://stackoverflow.com/questions/49480442/detecting-integer-constant-expressions-in-macros/49481840#49481840), or [this version](https://stackoverflow.com/a/49480926/501126) for C11+.
claunia commented 2026-01-29 20:40:14 +00:00
Author
Owner
Copy Link

@nemequ commented on GitHub (Apr 17, 2018):

Regarding __builtin_constant_p:

  • C11 macro:
    • IAR
    • SunCC
  • __builtin_constant_p
    • GCC 3.1+
    • Clang
    • ICC
    • TinyCC 0.9.19+
    • armcc 5.04+
  • Insane macro
    • TI
    • XL C/C++
  • Nothing
    • MSVC
    • DMC
@nemequ commented on GitHub (Apr 17, 2018): Regarding `__builtin_constant_p`: * C11 macro: * IAR * SunCC * `__builtin_constant_p` * GCC 3.1+ * Clang * ICC * TinyCC 0.9.19+ * armcc 5.04+ * Insane macro * TI * XL C/C++ * Nothing * MSVC * DMC
claunia commented 2026-01-29 20:40:14 +00:00
Author
Owner
Copy Link

@eustas commented on GitHub (Apr 18, 2018):

100% agree - all public macros should have specific BROTLI_ prefix. Going to fix soon.

Thanks!

@eustas commented on GitHub (Apr 18, 2018): 100% agree - all public macros should have specific `BROTLI_` prefix. Going to fix soon. Thanks!
claunia commented 2026-01-29 20:40:14 +00:00
Author
Owner
Copy Link

@eustas commented on GitHub (Apr 18, 2018):

Using Hedley / parts of Hedley would be nice, but, I'm afraid, "public domain" license is not allowed =(
Going to see if this could be overcome.

@eustas commented on GitHub (Apr 18, 2018): Using Hedley / parts of Hedley would be nice, but, I'm afraid, "public domain" license is not allowed =( Going to see if this could be overcome.
claunia commented 2026-01-29 20:40:14 +00:00
Author
Owner
Copy Link

@nemequ commented on GitHub (Apr 18, 2018):

Hedley is actually CC0, which is basically a legally-sound disclaimer of copyright to the maximum extent possible. Even if it's not possible to formally place works in the public domain in a given jurisdiction CC0 has a fallback which grants permission to use it for whatever you want. I'm not aware of any reason any project would be unable to use it (which is why I chose it), other than perhaps having to run everything by a legal department which just hasn't approved it yet. That said, I'm the sole author of Hedley right now; I could grant you permission to use it under a different license if need be.

If you just want to only use parts of it that's fine, please just change the namespace.

Anyways, defining __has_* is the real problem; I only noticed the other stuff because went a little overboard recently working on the same issues for Hedley… I'm sure the existing code works perfectly for the vast majority of users.

@nemequ commented on GitHub (Apr 18, 2018): Hedley is actually [CC0](https://creativecommons.org/publicdomain/zero/1.0/), which is basically a legally-sound disclaimer of copyright to the maximum extent possible. Even if it's not possible to formally place works in the public domain in a given jurisdiction CC0 has a fallback which grants permission to use it for whatever you want. I'm not aware of any reason any project would be unable to use it (which is why I chose it), other than perhaps having to run everything by a legal department which just hasn't approved it yet. That said, I'm the sole author of Hedley right now; I could grant you permission to use it under a different license if need be. If you just want to only use parts of it [that's fine](https://nemequ.github.io/hedley/#faq-partial), please just change the namespace. Anyways, defining `__has_*` is the real problem; I only noticed the other stuff because went a little overboard recently working on the same issues for Hedley… I'm sure the existing code works perfectly for the *vast* majority of users.
claunia commented 2026-01-29 20:40:15 +00:00
Author
Owner
Copy Link

@eustas commented on GitHub (Apr 20, 2018):

#662 deals with unprefixed macros in public headers. Going to pick Hedley as a replacement for "modern compiler" soon.

@eustas commented on GitHub (Apr 20, 2018): #662 deals with unprefixed macros in public headers. Going to pick Hedley as a replacement for "modern compiler" soon.
claunia commented 2026-01-29 20:40:15 +00:00
Author
Owner
Copy Link

@nemequ commented on GitHub (Apr 20, 2018):

If you want, I can provide a patch for that. I didn't want to rewrite Hedley in Brotli, but I don't mind using it…

@nemequ commented on GitHub (Apr 20, 2018): If you want, I can provide a patch for that. I didn't want to rewrite Hedley in Brotli, but I don't mind *using* it…
claunia commented 2026-01-29 20:40:15 +00:00
Author
Owner
Copy Link

@eustas commented on GitHub (Apr 20, 2018):

I think I'd pickup actually used parts of Hedley and change prefix. Of course with links, references and kudos to original project author =)

@eustas commented on GitHub (Apr 20, 2018): I think I'd pickup actually used parts of Hedley and change prefix. Of course with links, references and kudos to original project author =)
claunia commented 2026-01-29 20:40:15 +00:00
Author
Owner
Copy Link

@nemequ commented on GitHub (Apr 22, 2018):

The disadvantage of that is that it's harder to keep up to date with enhancements in Hedley since you can't just drop in a new version. If you're comfortable with that then I don't see a problem. I don't care about the credit (blame?), I just want to know about any bugs or improvements :)

Switching back to __builtin_constant_p, I put together something you can use. I'm also working on a version in the dev branch of Hedley. As you can see, it's evolved a bit to handle different use cases… IS_CONSTANT is probably the one you want, unless you've been misusing __builtin_constant_p. It will use __builtin_constant_p when available, and fall back on checking for integer constant expressions in C11 (except for suncc, which is broken) or on compilers which support sizeof(void).

@nemequ commented on GitHub (Apr 22, 2018): The disadvantage of that is that it's harder to keep up to date with enhancements in Hedley since you can't just drop in a new version. If you're comfortable with that then I don't see a problem. I don't care about the credit (blame?), I just want to know about any bugs or improvements :) Switching back to `__builtin_constant_p`, I put together [something you can use](https://github.com/nemequ/attic/blob/master/is_constant.h). I'm also working on a version in the [dev branch of Hedley](https://github.com/nemequ/hedley/tree/dev). As you can see, it's evolved a bit to handle different use cases… `IS_CONSTANT` is probably the one you want, unless you've been misusing `__builtin_constant_p`. It will use `__builtin_constant_p` when available, and fall back on checking for integer constant expressions in C11 (except for suncc, [which is broken](https://community.oracle.com/thread/4139563)) or on compilers which support `sizeof(void)`.
claunia commented 2026-01-29 20:40:15 +00:00
Author
Owner
Copy Link

@trudeaun commented on GitHub (Apr 25, 2018):

@nemequ Hi Evan, I noticed that you had mentioned XL in this issue and also in your comment on Stack Overflow. Sorry to hear that XL is causing you some trouble. Which of the following are you trying to achieve?

  1. Determine whether a macro's operand is an integer constant expression
  2. Determine whether the value of the macro's operand can be determined to be a constant
@trudeaun commented on GitHub (Apr 25, 2018): @nemequ Hi Evan, I noticed that you had mentioned XL in this issue and also in [your comment on Stack Overflow](https://stackoverflow.com/questions/49480442/detecting-integer-constant-expressions-in-macros/49481840#49481840). Sorry to hear that XL is causing you some trouble. Which of the following are you trying to achieve? 1. Determine whether a macro's operand is an integer constant expression 2. Determine whether the value of the macro's operand can be determined to be a constant
claunia commented 2026-01-29 20:40:15 +00:00
Author
Owner
Copy Link

@nemequ commented on GitHub (Apr 26, 2018):

@trudeaun the ICE macro works on XL C; I'm not sure what makes you think it's causing me trouble. I have a few issues with XL (which I've already reported on the community forum), but this isn't one of them…

Which of the following are you trying to achieve?

  1. Determine whether a macro's operand is an integer constant expression
  2. Determine whether the value of the macro's operand can be determined to be a constant

Both. I personally care about 2 a bit more (and I think that's what Brotli needs), since I use it for choosing between a code path which can be optimized away by the compiler if the value is known at compile-time and one which is fast at runtime, but 1 has its uses as well so I'd like to support it. https://github.com/nemequ/attic/blob/master/is_constant.h has a big comment at the beginning which may help clear things up a bit.

I'm happy to discuss this further, but we should probably do it in an issue in Hedley or my "attic" repo (or you can just e-mail me); it's a bit of a tangent for this issue.

@nemequ commented on GitHub (Apr 26, 2018): @trudeaun the ICE macro works on XL C; I'm not sure what makes you think it's causing me trouble. I have a few issues with XL (which I've already reported on the community forum), but this isn't one of them… > Which of the following are you trying to achieve? > > 1. Determine whether a macro's operand is an integer constant expression > 2. Determine whether the value of the macro's operand can be determined to be a constant Both. I personally care about 2 a bit more (and I think that's what Brotli needs), since I use it for choosing between a code path which can be optimized away by the compiler if the value is known at compile-time and one which is fast at runtime, but 1 has its uses as well so I'd like to support it. https://github.com/nemequ/attic/blob/master/is_constant.h has a big comment at the beginning which may help clear things up a bit. I'm happy to discuss this further, but we should probably do it in an issue in [Hedley](https://github.com/nemequ/hedley/issues) or [my "attic" repo](https://github.com/nemequ/attic/issues) (or you can just e-mail me); it's a bit of a tangent for this issue.
claunia commented 2026-01-29 20:40:16 +00:00
Author
Owner
Copy Link

@eustas commented on GitHub (Dec 15, 2021):

@nemequ
Hi, Evan. Is there is something actionable on this issue, or we could close it?

@eustas commented on GitHub (Dec 15, 2021): @nemequ Hi, Evan. Is there is something actionable on this issue, or we could close it?
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
bug
enhancement
feature
help wanted
pull-request
Mirrored from GitHub Pull Request
 release-v1.1
release-v1.1.1
release-v1.2
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/brotli#224
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?