Closed Bug 1171020 Opened 5 years ago Closed 5 years ago

nsMultiMixedConv.cpp: Value stored to 'aLen' is never read

Categories

(Core :: Networking, defect)

40 Branch
defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: Sylvestre, Assigned: sanchitkum, Mentored)

Details

(Keywords: clang-analyzer, Whiteboard: [good first bug][lang=C++])

Attachments

(1 file, 3 obsolete files)

I would like to work on this bug. Can somebody please assign it me?
Flags: needinfo?(sledru)
Sure, I will assign it to you once you proposed a patch.
Flags: needinfo?(sledru)
Value stored to 'aLen' is never read fixes.
Flags: needinfo?(sledru)
Attachment #8615920 - Flags: review?(mcmanus)
Thanks
Assignee: nobody → sanchitkum
Flags: needinfo?(sledru)
Comment on attachment 8615920 [details] [diff] [review]
bug1171020_valueNeverReadFixes.diff

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

this doesn't help the readability of that code. aLen is the length of the buffer which has just been adjusted by 2 (by decreasing the pointer) - it makes sense to keep the length in sync if this code should change. compilers are good about ignoring this stuff if it doesn't matter. I don't think we should be coding around the scanner d'jour at the cost of readability.

if you can suppress the compiler warning with an annotation I would accept that patch. I would also accept a comment saying to expect a compiler warning.
Attachment #8615920 - Flags: review?(mcmanus) → review-
Hi! I am a beginner. Can you please guide me on How to suppress the compiler warning with an annotation?
Will the approach mentioned here, (http://stackoverflow.com/questions/1486904/how-do-i-best-silence-a-warning-about-unused-variables), be appropriate?
Thanks.
Flags: needinfo?(sledru)
(void)aLen += 2; + a comment explaining that
could be enough but please check with clang analyzer.
Flags: needinfo?(sledru)
Added (void)aLen to suppress the compiler warning.
Needs to be checked with clang analyzer though.
Attachment #8615920 - Attachment is obsolete: true
Flags: needinfo?(sledru)
Attachment #8616080 - Flags: review?(mcmanus)
Attachment #8616080 - Flags: review?(mcmanus)
(In reply to Sanchit Kumar [:sanchitkum] from comment #8)
>[..]
> Needs to be checked with clang analyzer though.

it sounds like you haven't confirmed that it fixes what you want. I can't review that :)

let me know if I misread the comment.
(In reply to Sylvestre Ledru [:sylvestre] from comment #7)
> (void)aLen += 2; + a comment explaining that
> could be enough but please check with clang analyzer.

How do we check with clang analyzer? :)
We cannot do (void) aLen += 2; as it showed error message "invalid operands to binary expression ('void' and 'int')".

Have added a new statement aLen along with a comment, this helps in suppressing the compiler warning.
Attachment #8616080 - Attachment is obsolete: true
Attachment #8616376 - Flags: review?(sledru)
Comment on attachment 8616376 [details] [diff] [review]
bug1171020_valueNeverReadFixes.diff

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

Added a new statement aLen; along with a comment. This helps in suppressing the compiler warning.
Attachment #8616376 - Flags: review?(sledru) → review?(mcmanus)
OK, thanks for the information.
Flags: needinfo?(sledru)
Comment on attachment 8616376 [details] [diff] [review]
bug1171020_valueNeverReadFixes.diff

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

thanks.
you have added trailing whitespace, please fix that before checking in.
Attachment #8616376 - Flags: review?(mcmanus) → review+
Fixed the whitespace issue.
Attachment #8617076 - Flags: review?(mcmanus)
Comment on attachment 8617076 [details] [diff] [review]
bug1171020_valueNeverReadFixes.diff

You don't need to ask for a review for a whitespace change.
Attachment #8617076 - Flags: review?(mcmanus) → review+
Attachment #8616376 - Attachment is obsolete: true
Keywords: checkin-needed
Sanchit, sorry but we should just wontfix this patch then...
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
Okay. Thanks for the help Sylvestre and Patrick. :)
Again, sorry about that but I don't think you lost your time.

I can give you an other good first bug if you want.
Yes, I am interested in an another good first bug.
I learnt something new, so I am completely fine even though it didn't land. :)
Thanks.
You need to log in before you can comment on or make changes to this bug.