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)

RESOLVED FIXED in Firefox 41

Status

()

Core
General
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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

3 years ago
Flags: needinfo?(bgirard)
(Assignee)

Comment 1

3 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.
--enable-debug?
(Assignee)

Comment 3

3 years ago
Created attachment 8624758 [details] [diff] [review]
fix v1

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

3 years ago
Flags: needinfo?(bgirard)
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

3 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

3 years ago
(Setting as blocking bug 793735, since this is fixing a typo in that bug's patch.)
Blocks: 793735
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

3 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

3 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

3 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
(Assignee)

Comment 12

3 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

3 years ago
(Also, FWIW: I sanity-checked that there are no other naked LINUX usages within /mozglue.)
https://hg.mozilla.org/mozilla-central/rev/bcf7d4b06871
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.