Closed Bug 1247301 Opened 4 years ago Closed 4 years ago

[Brotli] Warning in bit_reader.h: use of logical '||' with constant operand

Categories

(Core :: Graphics: Text, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: ckerschb, Assigned: fredw)

References

Details

(Whiteboard: gfx-noted)

Attachments

(2 files)

No description provided.
 5:22.18 /home/ckerschb/moz/mc-obj-dbg/dist/include/./bit_reader.h:41:22: error: use of logical '||' with constant operand [-Werror,-Wconstant-logical-operand]
 5:22.18   if (IS_CONSTANT(n) || BROTLI_HAS_UBFX) {
 5:22.18                      ^  ~~~~~~~~~~~~~~~
 5:22.18 /home/ckerschb/moz/mc-obj-dbg/dist/include/./bit_reader.h:41:22: note: use '|' for a bitwise operation
 5:22.18   if (IS_CONSTANT(n) || BROTLI_HAS_UBFX) {
 5:22.18                      ^~
 5:22.18                      |
 5:22.18 1 error generated.
Blocks: 1242904
Attachment #8717932 - Flags: review?(mcmanus) → review?(fred.wang)
Whiteboard: gfx-noted
Comment on attachment 8717932 [details] [diff] [review]
bug_1247301_fix_build_error.patch

mmh, I saw this build warning too, but the name of the defines really suggests that the Brotli authors wanted standard boolean OR not bitwise OR ; so I didn't patch it.

Here are the relevant definitions from port.h:

#if defined(BROTLI_TARGET_ARM)
#define BROTLI_HAS_UBFX 1
#else
#define BROTLI_HAS_UBFX 0
#endif

#if BROTLI_MODERN_COMPILER || __has_builtin(__builtin_constant_p)
#define IS_CONSTANT(x) __builtin_constant_p(x)
#else
#define IS_CONSTANT(x) 0
#endif

and __builtin_constant_p returns 0 or 1 per the GCC documentation.

So I think the proposed patch will work, but there might be a way to rewrite this while preserving the semantics implied by ||. Could you please report this upstream to see how they prefer to fix that?
(In reply to Frédéric Wang (:fredw) from comment #3)
> Comment on attachment 8717932 [details] [diff] [review]
> bug_1247301_fix_build_error.patch
> 
> mmh, I saw this build warning too, but the name of the defines really
> suggests that the Brotli authors wanted standard boolean OR not bitwise OR ;
> so I didn't patch it.
> 
> Here are the relevant definitions from port.h:
> 
> #if defined(BROTLI_TARGET_ARM)
> #define BROTLI_HAS_UBFX 1
> #else
> #define BROTLI_HAS_UBFX 0
> #endif
> 
> #if BROTLI_MODERN_COMPILER || __has_builtin(__builtin_constant_p)
> #define IS_CONSTANT(x) __builtin_constant_p(x)
> #else
> #define IS_CONSTANT(x) 0
> #endif
> 
> and __builtin_constant_p returns 0 or 1 per the GCC documentation.
> 
> So I think the proposed patch will work, but there might be a way to rewrite
> this while preserving the semantics implied by ||. Could you please report
> this upstream to see how they prefer to fix that?

Fred, I would prefer you can take care of that - I just realized the build error.
Flags: needinfo?(fred.wang)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> Fred, I would prefer you can take care of that - I just realized the build
> error.

OK, I will.
Flags: needinfo?(fred.wang)
(In reply to Frédéric Wang (:fredw) from comment #5)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> > Fred, I would prefer you can take care of that - I just realized the build
> > error.
> 
> OK, I will.

Thanks!
Comment on attachment 8717932 [details] [diff] [review]
bug_1247301_fix_build_error.patch

I'm clearing review flag. Just to repeat, I believe the propose changed will fix the compilation warning and will provide the expected execution behavior. However, as I understand, -Wconstant-logical-operand is supposed to prevent people from doing logical operations with constant bit mask, instead of bitwise operations. That does not apply here since the constants in question are really 0 and 1 used to represent boolean values false and true.
Attachment #8717932 - Flags: review?(fred.wang)
Summary: Fix build error from Bug 1242904 → [Brotli] Warning in bit_reader.h: use of logical '||' with constant operand
Attached patch PatchSplinter Review
So the upstream fix will be to use double exclamation marks to force these values to be interpreted as booleans. This is more appropriate than changing the semantic of the operator.
Assignee: nobody → fred.wang
Status: NEW → ASSIGNED
Attachment #8723524 - Flags: review?(mcmanus)
Comment on attachment 8723524 [details] [diff] [review]
Patch

Review of attachment 8723524 [details] [diff] [review]:
-----------------------------------------------------------------

please note in the readme.mozilla (or whatever its called) that this upstream patch has been imported and we're not quite in sync with the cset mentioned in that file
Attachment #8723524 - Flags: review?(mcmanus) → review+
https://hg.mozilla.org/mozilla-central/rev/6016ec162137
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Blocks: 1280047
You need to log in before you can comment on or make changes to this bug.