1176266 - 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)

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Daniel Holbert [:dholbert]]

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?

Comment 1

[Daniel Holbert [:dholbert]]

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

[Benoit Girard (:BenWa)]

--enable-debug?

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: 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

Comment 4

[Benoit Girard (:BenWa)]

Comment on attachment 8624758 [details] [diff] [review]
fix v1

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

I'm comfortable reviewing this.

Comment 5

[Daniel Holbert [:dholbert]]

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.

Comment 6

[Daniel Holbert [:dholbert]]

(Setting as blocking bug 793735, since this is fixing a typo in that bug's patch.)

Comment 7

[Benoit Girard (:BenWa)]

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.

Comment 8

[Daniel Holbert [:dholbert]]

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

Comment 9

[Daniel Holbert [:dholbert]]

(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.

Comment 10

[Daniel Holbert [:dholbert]]

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.)

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcf7d4b06871

Comment 12

[Daniel Holbert [:dholbert]]

(And presumably we were OK without the <string.h> include include before this, simply due to unified builds pulling it in from another file.)

Comment 13

[Daniel Holbert [:dholbert]]

(Also, FWIW: I sanity-checked that there are no other naked LINUX usages within /mozglue.)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bcf7d4b06871