1130244 - Fix clang warnings in nsprpub

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: nspr-clang-warnings.patch

I see the following clang warnings when building Firefox on OS X:

> nsprpub/pr/src/io/prfdcach.c:216:18 [-Wunused-variable] unused variable 'pop'

My patch removes the unused variable `pop`.

> nsprpub/pr/src/misc/prenv.c:64:29 [-Wincompatible-pointer-types-discards-qualifiers] passing 'const char *' to parameter of type 'char *' discards qualifiers

My patch casts away the const-ness of PR_SetEnv()'s parameter because putenv() takes a non-const char* on Linux and OS X. I also cleaned up some whitespace in prenv.c.

> nsprpub/pr/src/pthreads/ptthread.c:683:12 [-Wunused-variable] unused variable 'rv'

My patch removes the initialization of `rv` (because it is redundant in the #ifdef'd code paths that use rv) and adds a (void)rv cast to suppress the -Wunused-variable warning only in the #else code path.

Comment 1

[:Cykesiopka]

(In reply to Chris Peterson [:cpeterson] from comment #0)
> > nsprpub/pr/src/io/prfdcach.c:216:18 [-Wunused-variable] unused variable 'pop'
> 
> My patch removes the unused variable `pop`.

FWIW this was already fixed by Bug 1126408; The in tree copy of NSPR hasn't been updated to pull in the fix though.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: nspr-clang-warnings-v2.patch

patch v2 excludes the -Wunused-variable warning about 'pop' and just fixes these two clang warnings:

nsprpub/pr/src/misc/prenv.c:64:29 [-Wincompatible-pointer-types-discards-qualifiers] passing 'const char *' to parameter of type 'char *' discards qualifiers

nsprpub/pr/src/pthreads/ptthread.c:683:12 [-Wunused-variable] unused variable 'rv'

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: more-clang-warnings.patch

Part 2: Fix more clang warnings in NSPR

nsprpub/pr/src/pthreads/ptsynch.c:325:13 [-Wunused-variable] unused variable 'rv'
nsprpub/pr/src/pthreads/ptsynch.c:340:16 [-Wunused-variable] unused variable 'rv'
nsprpub/pr/src/pthreads/ptthread.c:683:12 [-Wunused-variable] unused variable 'rv'

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: part-2-ptsynch-warnings-v2.patch

patch 2 v2: oops. My patch inadvertently reversed the (0 == rv) check logic.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 8562574 [details] [diff] [review]
nspr-clang-warnings-v2.patch

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

::: nsprpub/pr/src/misc/prenv.c
@@ +62,3 @@
>  
>      _PR_LOCK_ENV();
> +    result = _PR_MD_PUT_ENV((char*)string);

I was going to complain about this possibly being harmful, but I realize that we're already disregarding constness here, so this is just silencing the warning.

Comment 6

[Chris Peterson [:cpeterson]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
> I was going to complain about this possibly being harmful, but I realize
> that we're already disregarding constness here, so this is just silencing
> the warning.

Yes, this function is dangerous because putenv() retains the string pointer, so the caller should not modify or free it. setenv() would be safer. :(

Comment 7

[Chris Peterson [:cpeterson]]

Ted, I don't have NSPR commit privileges. Should I mark this bug checkin-needed or can you land my patches?

Comment 8

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/projects/nspr/rev/0553227067d9
https://hg.mozilla.org/projects/nspr/rev/6b30fde4148a

Comment 9

[(not currently active) Ted Mielczarek]

Sorry for the delay! In the future, could you attach patches against the NSPR repo and with the commit message finalized for me to commit? Takes a few steps out of the process for me. Thanks for the patches!