Make setting NULL to an outparam on failure ok

RESOLVED FIXED

Status

()

Core
Rewriting and Analysis
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Benjamin Smedberg, Assigned: dmandelin)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
It turns out that we have lots of code that assumes setting an outparam to NULL on failure is ok. So, rather than try to rewrite the world, I'd like to suppress this class of warning... I have a trivial patch to do that, but it turns out there are some edge cases that need tightening up... I'll attach a testcase shortly.
(Reporter)

Comment 1

9 years ago
Created attachment 334320 [details] [diff] [review]
Ignore NULL outparams, rev. 1
Attachment #334320 - Flags: review?(dmandelin)
(Assignee)

Updated

9 years ago
Attachment #334320 - Flags: review?(dmandelin) → review+
(Reporter)

Comment 2

9 years ago
Created attachment 334326 [details] [diff] [review]
Null pattern needing tweaking, rev. 1

This testcase is the fairly common null pattern that needs tweaking. Basically, if you explicitly test the value of the outparam, the state machine should treat is as a NULL-set variable.
(Reporter)

Comment 3

9 years ago
Pushed the first part: http://hg.mozilla.org/mozilla-central/index.cgi/rev/1962de2d4e92
(Assignee)

Comment 4

9 years ago
Created attachment 334353 [details] [diff] [review]
Patch for onull.cpp test case

This should take care of it. By the way, e3.cpp should now succeed. Should we just rename it to onull2.cpp and move it to that test category.
(Reporter)

Comment 5

9 years ago
Comment on attachment 334353 [details] [diff] [review]
Patch for onull.cpp test case

Looks good. Yes, go ahead and do the file-move, and I think there's an extraneous FLOW_PASS_TESTCASES in the patch.
Attachment #334353 - Flags: review+
(Assignee)

Comment 6

9 years ago
Created attachment 334524 [details] [diff] [review]
Patch for checkin
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(Assignee)

Updated

9 years ago
Attachment #334353 - Attachment is obsolete: true
(Assignee)

Comment 7

9 years ago
Created attachment 334526 [details]
Bundle version of onull2.diff
(Reporter)

Comment 8

9 years ago
pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/3390347493fd
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Reporter)

Comment 9

9 years ago
You forgot to add/rename the testcases in the previous commit. I've fixed this on mozilla-central with the following commit:

http://hg.mozilla.org/mozilla-central/rev/a4d2e6549f7f
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.