Fix clang warnings in nsprpub

RESOLVED FIXED

Status

NSPR
NSPR
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

(Blocks: 1 bug)

4.10.9
x86
Mac OS X

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8560217 [details] [diff] [review]
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.
Attachment #8560217 - Flags: review?(wtc)

Comment 1

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

Comment 2

3 years ago
Created attachment 8562574 [details] [diff] [review]
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'
Assignee: wtc → cpeterson
Attachment #8560217 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8560217 - Flags: review?(wtc)
Attachment #8562574 - Flags: review?(ted)
(Assignee)

Comment 3

3 years ago
Created attachment 8563939 [details] [diff] [review]
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'
Attachment #8563939 - Flags: review?(ted)
(Assignee)

Comment 4

3 years ago
Created attachment 8564774 [details] [diff] [review]
part-2-ptsynch-warnings-v2.patch

patch 2 v2: oops. My patch inadvertently reversed the (0 == rv) check logic.
Attachment #8563939 - Attachment is obsolete: true
Attachment #8563939 - Flags: review?(ted)
Attachment #8564774 - Flags: review?(ted)
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.
Attachment #8562574 - Flags: review?(ted) → review+
Attachment #8564774 - Flags: review?(ted) → review+
(Assignee)

Comment 6

3 years ago
(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. :(
(Assignee)

Comment 7

3 years ago
Ted, I don't have NSPR commit privileges. Should I mark this bug checkin-needed or can you land my patches?
Flags: needinfo?(ted)
https://hg.mozilla.org/projects/nspr/rev/0553227067d9
https://hg.mozilla.org/projects/nspr/rev/6b30fde4148a
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!
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(ted)
Resolution: --- → FIXED
Version: other → 4.10.9
You need to log in before you can comment on or make changes to this bug.