Closed
Bug 1176266
Opened 9 years ago
Closed 9 years ago
TimeStamp_posix.cpp:57:23: error: unused variable 'kNsPerUs' [-Werror,-Wunused-const-variable] (due in part to incorrect #if check for LINUX instead of XP_LINUX)
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: dholbert, Assigned: dholbert)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
Hitting this local build error (in build with clang 3.7 and --enable-warnings-as-errors, on linux):
{
mozglue/misc/TimeStamp_posix.cpp:57:23: error: unused variable 'kNsPerUs' [-Werror,-Wunused-const-variable]
0:02.72 static const uint16_t kNsPerUs = 1000;
}
Looks like this variable *is* technically used, but its usages are behind #ifdefs.
I'm tentatively blaming bug 858927, since it just touched this file. (moving it to its current location. (Maybe it was already a warning, and is now treated as an error because this file has been moved from a non-FAIL_ON_WARNINGS directory to a FAIL_ON_WARNINGS directory? Not sure.)
BenWa, do you see this warning (error) locally? Mind taking a look?
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(bgirard)
Assignee | ||
Comment 1•9 years ago
|
||
At first glance, it looks like the relevant #ifdef is:
#if defined(LINUX) || defined(ANDROID)
This check *fails* on my linux machine (as in, we skip all the code that's protected by it). I think it maybe wants to be XP_LINUX?
That's not all we need here, though. If I change the condition to use XP_LINUX, then I get this build error:
{
TimeStamp_posix.cpp:233:13: error: use of undeclared identifier 'strrchr'
}
To fix that, it looks like we just need to #include string.h. I'll post a patch to do that, coming up.
Comment 2•9 years ago
|
||
--enable-debug?
Assignee | ||
Comment 3•9 years ago
|
||
This patch lets me build again. BenWa, do you want to review?
FWIW: Looks like the buggy "LINUX" reliance here came from bug 793735:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dab96a956516#l3.106
Attachment #8624758 -
Flags: feedback?(bgirard)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(bgirard)
Comment 4•9 years ago
|
||
Comment on attachment 8624758 [details] [diff] [review]
fix v1
Review of attachment 8624758 [details] [diff] [review]:
-----------------------------------------------------------------
I'm comfortable reviewing this.
Attachment #8624758 -
Flags: feedback?(bgirard) → review+
Assignee | ||
Comment 5•9 years ago
|
||
Thanks!
Note that this patch will change behavior, since Linux will now actually be getting the code that bug 793735 intended for it to get. I'll give it a try run before pushing.
Assignee: nobody → dholbert
Summary: TimeStamp_posix.cpp:57:23: error: unused variable 'kNsPerUs' [-Werror,-Wunused-const-variable] → TimeStamp_posix.cpp:57:23: error: unused variable 'kNsPerUs' [-Werror,-Wunused-const-variable] (due in part to incorrect #if check for LINUX instead of XP_LINUX)
Assignee | ||
Comment 6•9 years ago
|
||
(Setting as blocking bug 793735, since this is fixing a typo in that bug's patch.)
Blocks: 793735
Comment 7•9 years ago
|
||
Good point, but I'm assuming that the review from bug 858927 already reviewed that.
Either something #define LINUX locally in xpcom and my patch temporarily changed the behavior or it never worked as intended.
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #2)
> --enable-debug?
(Sorry, just saw this.) Yeah, my mozconfig here was simply:
ac_add_options --enable-debug --disable-optimize
ac_add_options --enable-profiling
ac_add_options --enable-warnings-as-errors
And this was actually clang 3.6, not 3.7. (not that it matters too much)
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #7)
> Either something #define LINUX locally in xpcom and my patch temporarily
> changed the behavior or it never worked as intended.
I'm assuming it's the latter; testing that theory right now.
Assignee | ||
Comment 10•9 years ago
|
||
Nope, looks like it's the former (something did indeed #define LINUX locally in xpcom and your patch temporarily changed behavior).
At least, if I add a single line "donotbuild" after #if defined(LINUX) || defined(ANDROID), after updating to the cset right before your push (325631a7b72a), I get a build error "error: unknown type name 'donotbuild'"
If I do that in current mozilla-inbound with this file in its new location, I don't get a build error (instead, I get the build warning treated as an error described in comment 0.)
No longer blocks: 793735
Comment 11•9 years ago
|
||
Assignee | ||
Comment 12•9 years ago
|
||
(And presumably we were OK without the <string.h> include include before this, simply due to unified builds pulling it in from another file.)
Assignee | ||
Comment 13•9 years ago
|
||
(Also, FWIW: I sanity-checked that there are no other naked LINUX usages within /mozglue.)
Comment 14•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•