Closed
Bug 1298728
Opened 8 years ago
Closed 8 years ago
Annotate some non-obvious case fallthroughs in the HTML parser.
Categories
(Core :: DOM: HTML Parser, defect)
Core
DOM: HTML Parser
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
(Blocks 1 open bug)
Details
(Keywords: coverity, Whiteboard: [CID 748955, CID 748956])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
hsivonen
:
feedback-
|
Details | Diff | Splinter Review |
nsHtml5TreeBuilder has a couple of case fallthroughs that Coverity finds suspicious. We should make the intent clearer here.
Assignee | ||
Comment 1•8 years ago
|
||
I have optimistically assumed that the current code is correct, and this patch is just making it clearer. Please check that assumption! I'd be happy if this had discovered a genuine bug.
Attachment #8785758 -
Flags: review?(hsivonen)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
I missed one case. I also now see that this is a generated file. hsivonen can you please check whether these fallthroughs are ok? If so, then I'll just mark the bug WONTFIX and mark them as false positives in Coverity. (If not, we can work out how to fix the Java code.)
Attachment #8785759 -
Flags: feedback?(hsivonen)
Assignee | ||
Updated•8 years ago
|
Attachment #8785758 -
Attachment is obsolete: true
Attachment #8785758 -
Flags: review?(hsivonen)
Comment on attachment 8785759 [details] [diff] [review] Annotate some non-obvious case fallthroughs in the HTML parser Indeed, this is generated code, so these annotations would be overwritten. Hence, feedback-. All these cases are documented as comments in the Java source. We should probably change those comments to something like // CPPONLY: fallThrough(); and then change the translator to special case a method invocation fallThrough() to emit C++ macro invocation that we could then define as expanding to whatever incantations each compiler or static analysis tool wants to see. ("// CPPONLY:" already gets removed by a preprecessor before translation, so that kind of Java comments allow Java code seen by the translator but not seen by the normal Java compiler.)
Attachment #8785759 -
Flags: feedback?(hsivonen) → feedback-
Note that there are lots of intentional (commented in the Java source) fall-throughs in the tokenizer. Hence, https://dxr.mozilla.org/mozilla-central/source/parser/html/moz.build#104
Assignee | ||
Comment 5•8 years ago
|
||
> All these cases are documented as comments in the Java source. We should
> probably change those comments to something like
> // CPPONLY: fallThrough();
> and then change the translator to special case a method invocation
> fallThrough() to emit C++ macro invocation that we could then define as
> expanding to whatever incantations each compiler or static analysis tool
> wants to see.
I don't think that's worth the effort. I've marked the reports in Coverity as intentional. Thank you for the feedback.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Updated•6 years ago
|
Blocks: coverity-analysis
You need to log in
before you can comment on or make changes to this bug.
Description
•