Remove useless null checks after allocating memory with |new| from xpcom/threads/

RESOLVED FIXED in Firefox 40

Status

()

Core
XPCOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: froydnj, Assigned: atifhans, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla40
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8587484 [details] [diff] [review]
Removes useless null checks after allocating from |new| from the files in xpcom/threads directory.

This is the fresh patch with changes suggested by Nathan. Only checks after explicit allocation from new were removed. I tried to trace through the function calls for the rest of the cases, but couldn't find initial allocation by new.
Attachment #8587484 - Flags: review?(nfroyd)
(Assignee)

Comment 2

3 years ago
Also while generating the patch for new changes, the new patch file stores the incremental diffs only. Is there a way to generate a patch file which incorporates all the changes i have made to my source tree till now. For the previous patch i had to use hg update --clean and redo my changes to generate the patch from start.
(Reporter)

Comment 3

3 years ago
Comment on attachment 8587484 [details] [diff] [review]
Removes useless null checks after allocating from |new| from the files in xpcom/threads directory.

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

Looks great, thank you!
Attachment #8587484 - Flags: review?(nfroyd) → review+
(Reporter)

Comment 4

3 years ago
(In reply to Atif Iqbal Ahangar [:atifhans] from comment #2)
> Also while generating the patch for new changes, the new patch file stores
> the incremental diffs only. Is there a way to generate a patch file which
> incorporates all the changes i have made to my source tree till now. For the
> previous patch i had to use hg update --clean and redo my changes to
> generate the patch from start.

How were you generating the patch file in the first place, before you made your changes to nsEnvironment.cpp?  Were you using |hg commit| or something else?
Flags: needinfo?(atif.hans)
(Assignee)

Comment 5

3 years ago
I just followed the steps from codefirefox.com video http://codefirefox.com/video/patch-changes. The command I used was:
|hg qnew bug1150197_remove_useless_null_checks_in_xpcom_thread.diff -m "Bug 1150197 - Remove useless  null checks after allocating memory with new from xpcom/threads/"|

It also mentions about |hg qref| which updates previous patch file with new changes, but in my case I had to create a new patch file with all the changes.

Maybe I will read a tutorial about Mercurial, I am using it for the first time.
Flags: needinfo?(atif.hans)
(Reporter)

Comment 6

3 years ago
(In reply to Atif Iqbal Ahangar [:atifhans] from comment #5)
> I just followed the steps from codefirefox.com video
> http://codefirefox.com/video/patch-changes. The command I used was:
> |hg qnew bug1150197_remove_useless_null_checks_in_xpcom_thread.diff -m "Bug
> 1150197 - Remove useless  null checks after allocating memory with new from
> xpcom/threads/"|

Ah, OK.

> It also mentions about |hg qref| which updates previous patch file with new
> changes, but in my case I had to create a new patch file with all the
> changes.

Yes, |hg qref| is what you want to use in this case: the workflow would look something like:

1. Make some changes.
2. hg qnew no-null-checks.patch -m "..."
3. Make some more changes.
4. hg qref

and then no-null-checks.patch will be updated with the changes you made in step 3.
(Assignee)

Comment 7

3 years ago
I don't have commit access and edit bug permissions. Can you check in this for me.
(Assignee)

Comment 8

3 years ago
(In reply to Atif Iqbal Ahangar [:atifhans] from comment #7)
> I don't have commit access and edit bug permissions. Can you check in this
> for me.
Flags: needinfo?(nfroyd)
Keywords: checkin-needed
(Reporter)

Comment 9

3 years ago
Checked in:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1748feaa98e2

The needinfo was sufficient for this. :)
Flags: needinfo?(nfroyd) → in-testsuite-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1748feaa98e2
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.