security/sandbox/chromium/base/time/time.h:690:37: error: possible misuse of comma operator here [-Werror,-Wcomma]

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

Filing this bug for a build warning that I'm getting (many times over) from a chromium header that gets included in various places.

I'm using clang 5.0 Trunk (built from svn 304217), and the build warning output looks like this (for every file that indirectly includes this "time.h" header:

==========
 2:57.37 In file included from /scratch/work/builds/mozilla-inbound/mozilla/security/sandbox/chromium-shim/base/threading/platform_thread_linux.cpp:11:
 2:57.37 In file included from /scratch/work/builds/mozilla-inbound/mozilla/security/sandbox/chromium/base/threading/platform_thread.h:16:
 2:57.37 /scratch/work/builds/mozilla-inbound/mozilla/security/sandbox/chromium/base/time/time.h:690:37: error: possible misuse of comma operator here [-Werror,-Wcomma]
 2:57.37           DCHECK(positive_value > 0),
 2:57.37                                     ^
 2:57.37 /scratch/work/builds/mozilla-inbound/mozilla/security/sandbox/chromium/base/time/time.h:690:11: note: cast expression to void to silence warning
 2:57.37           DCHECK(positive_value > 0),
 2:57.37           ^~~~~~~~~~~~~~~~~~~~~~~~~~
 2:57.37           static_cast<void>(        )
 2:57.37 /scratch/work/builds/mozilla-inbound/mozilla/security/sandbox/chromium/base/logging.h:702:3: note: expanded from macro 'DCHECK'
 2:57.37   LAZY_STREAM(LOG_STREAM(DCHECK), DCHECK_IS_ON() ? !(condition) : false) \
 2:57.38   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2:57.38 /scratch/work/builds/mozilla-inbound/mozilla/security/sandbox/chromium/base/logging.h:364:3: note: expanded from macro 'LAZY_STREAM'
 2:57.38   !(condition) ? (void) 0 : ::logging::LogMessageVoidify() & (stream)
 2:57.38   ^
 2:57.43 1 error generated.
==========

We can work around this pretty easily by taking clang's suggestion and adding a (void) cast to suppress the warning.

I've filed a bug upstream:
  https://bugs.chromium.org/p/chromium/issues/detail?id=729123
but we might as well also fix this locally too. It looks like we've taken some other spot-fixes to address issues in this chromium subtree checkout, at least.
Looks like this Chromium code is using the comma so DCHECK is part of the return expression instead of a statement so that the TimeDelta::FromProduct function will be constexpr.
Assignee: nobody → dholbert
(Bob, would you be the right person to review this?  I'm guessing so, given bug 1366701 / bug 1337331, but feel free to redirect the review request if needed.)
Blocks: buildwarning
Comment on attachment 8873965 [details]
Bug 1369837: Add a void cast to silence clang Wcomma build warning, in sandbox's snapshot of chromium header.

https://reviewboard.mozilla.org/r/145360/#review149384

Given that this is filed upstream let's go with this (thanks for doing that :-) ).

If for some reason that gets rejected then I might look to ignore this error in this code instead.
Attachment #8873965 - Flags: review?(bobowencode) → review+
(In reply to Bob Owen (:bobowen) from comment #4)
> If for some reason that gets rejected then I might look to ignore this error
> in this code instead.

FWIW, I started out by trying to make us ignore this warning (suppressing it in moz.build files), but I gave up after I'd fixed two different moz.build files and was still getting new instances of this failure for other #includers of the header.

(This header is included (directly or indirectly) in multiple source directories with their own moz.build files, so we would need to suppress the warning in each of those client moz.build files. Not sure precisely how many, but "more than two" made this feel like the better solution for the time being).

Thanks for the review!
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/85f41e26f8b6
Add a void cast to silence clang Wcomma build warning, in sandbox's snapshot of chromium header. r=bobowen
https://hg.mozilla.org/mozilla-central/rev/85f41e26f8b6
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
For the record: the Chromium bug report was updated today...
 https://bugs.chromium.org/p/chromium/issues/detail?id=729123#c5
...to note that they landed a patch for this back in September:
 https://chromium.googlesource.com/chromium/src/+/fb3a33751c190240b64a1b22feee6704c2a822bc%5E%21/#F0


Their patch pulled the "DCHECK(positive_value > 0)" statement out of the return statement, and removed the #ifdefs around it (which were just to special case a particular ~old MSVC version).

I don't think we need to bother updating our hackaround to match theirs at this point; we can just let our local hackaround get overwritten when we next synchronize like Bug 1337331.
You need to log in before you can comment on or make changes to this bug.