Closed
Bug 1168033
Opened 9 years ago
Closed 9 years ago
Value stored to 'attemptedOptimisticPipeline' is never read in nsHttpConnectionMgr::TryDispatchTransaction
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla43
People
(Reporter: Sylvestre, Assigned: m.fornaro6, Mentored)
Details
(Keywords: clang-analyzer, Whiteboard: [good first bug][lang=C++])
Attachments
(1 file, 4 obsolete files)
949 bytes,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
Dead initialization. See scan-build report at: https://people.mozilla.org/~sledru/reports/fx-scan-build/report-nsHttpConnectionMgr.cpp-TryDispatchTransaction-146-1.html#EndPath Value stored to 'attemptedOptimisticPipeline' is never read in nsHttpConnectionMgr::TryDispatchTransaction. Line 1880
Attachment #8610178 -
Flags: review+
Reporter | ||
Comment 3•9 years ago
|
||
Comment on attachment 8610178 [details] [diff] [review] Bug1168033.patch: remove unused initialization You have to find a reviewer for your patch and, in this case, use the option "?". The easiest way to find one is to look at the hg history of this file.
Attachment #8610178 -
Flags: review+
Attachment #8610178 -
Flags: review?(mcmanus)
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → m.fornaro6
Comment 5•9 years ago
|
||
Comment on attachment 8610178 [details] [diff] [review] Bug1168033.patch: remove unused initialization Review of attachment 8610178 [details] [diff] [review]: ----------------------------------------------------------------- that state change is there in case it is used in the conditional for any subsequent step (so the state is reflected accurately). you can comment it if you like.
Attachment #8610178 -
Flags: review?(mcmanus) → review-
(In reply to Patrick McManus [:mcmanus] from comment #5) > Comment on attachment 8610178 [details] [diff] [review] > Bug1168033.patch: remove unused initialization > > Review of attachment 8610178 [details] [diff] [review]: > ----------------------------------------------------------------- > > that state change is there in case it is used in the conditional for any > subsequent step (so the state is reflected accurately). > > you can comment it if you like. I don't know if I understood the problem correctly, as Sylvestre pointed out 'attemptedOptimisticPipeline' is never read after that inizialization, and it is a local variable. So that initialization seem to be useless.
Flags: needinfo?(sledru)
Flags: needinfo?(mcmanus)
(In reply to Patrick McManus [:mcmanus] from comment #5) > you can comment it if you like. Sorry, I didn't saw "it" in yout phrase. I'll comment the line.
Comment 8•9 years ago
|
||
(In reply to Marco from comment #6) > (In reply to Patrick McManus [:mcmanus] from comment #5) > > Comment on attachment 8610178 [details] [diff] [review] > > Bug1168033.patch: remove unused initialization > > > > Review of attachment 8610178 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > that state change is there in case it is used in the conditional for any > > subsequent step (so the state is reflected accurately). > > > > you can comment it if you like. > > I don't know if I understood the problem correctly, > as Sylvestre pointed out 'attemptedOptimisticPipeline' is never read after > that inizialization, and it is a local variable. So that initialization seem > to be useless. yes, that assignment (it isn't the initialization) has no code impact - its essentially documentation of the pattern already established in the function. The compiler can remove the work. The idea is that new steps can be added and that variable is part of the state they can count on.
Flags: needinfo?(mcmanus)
Reporter | ||
Comment 9•9 years ago
|
||
Just what Patrick said. Sometimes, it is interesting to keep useless declarations in the code as hint for the developers.
Flags: needinfo?(sledru)
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to PTO - Patrick McManus [:mcmanus] from comment #8) > (In reply to Marco from comment #6) > > (In reply to Patrick McManus [:mcmanus] from comment #5) > > > Comment on attachment 8610178 [details] [diff] [review] > > > Bug1168033.patch: remove unused initialization > > > > > > Review of attachment 8610178 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > that state change is there in case it is used in the conditional for any > > > subsequent step (so the state is reflected accurately). > > > > > > you can comment it if you like. > > > > I don't know if I understood the problem correctly, > > as Sylvestre pointed out 'attemptedOptimisticPipeline' is never read after > > that inizialization, and it is a local variable. So that initialization seem > > to be useless. > > yes, that assignment (it isn't the initialization) has no code impact - its > essentially documentation of the pattern already established in the > function. The compiler can remove the work. The idea is that new steps can > be added and that variable is part of the state they can count on. Ok...so? If it is there for documentation purpouse I also think the compiler is smart enough to strip it out. So we can just close the bug?
Reporter | ||
Comment 11•9 years ago
|
||
Or "you can comment it if you like." as Patrick offered.
Assignee | ||
Comment 12•9 years ago
|
||
Commented assignment instead of deleting
Attachment #8610178 -
Attachment is obsolete: true
Attachment #8613281 -
Flags: review?(mcmanus)
Comment 13•9 years ago
|
||
Comment on attachment 8613281 [details] [diff] [review] Bug1168033.patch: remove unused initialization V2 Review of attachment 8613281 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp @@ +1875,5 @@ > if (!attemptedOptimisticPipeline && > (classification == nsHttpTransaction::CLASS_REVALIDATION || > classification == nsHttpTransaction::CLASS_SCRIPT)) { > + // Commented because not read after assignment > + //attemptedOptimisticPipeline = true; please don't change this. just add a comment.
Attachment #8613281 -
Flags: review?(mcmanus) → review-
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8613281 -
Attachment is obsolete: true
Attachment #8613920 -
Flags: review?(mcmanus)
Assignee | ||
Comment 15•9 years ago
|
||
Sorry, I've some problem with english and I'm not familiar with your way of working. I really don't understand if you want me to comment (read "remove") that line or comment (read "add a description"). In doubt I've added both patch. I hope to do better the next time :)
Attachment #8613927 -
Flags: review?(mcmanus)
Assignee | ||
Comment 16•9 years ago
|
||
used space for identation
Attachment #8613927 -
Attachment is obsolete: true
Attachment #8613927 -
Flags: review?(mcmanus)
Attachment #8613933 -
Flags: review?(mcmanus)
Comment 17•9 years ago
|
||
Comment on attachment 8613933 [details] [diff] [review] Bug1168033.patch: remove unused initialization V4 Review of attachment 8613933 [details] [diff] [review]: ----------------------------------------------------------------- thank you
Attachment #8613933 -
Flags: review?(mcmanus) → review+
Updated•9 years ago
|
Attachment #8613920 -
Flags: review?(mcmanus) → review-
Reporter | ||
Comment 18•9 years ago
|
||
This stuff never landed. Requesting it. The patch still apply cleaning: "Hunk #1 succeeded at 1811 (offset -59 lines)."
Keywords: checkin-needed
Updated•9 years ago
|
Attachment #8613920 -
Attachment is obsolete: true
Comment 19•9 years ago
|
||
In the future, please ensure that patches include commit information so they're ready for checking in once they receive proper review. https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Comment 20•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b819a195143
Keywords: checkin-needed
Comment 21•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2b819a195143
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•