Closed Bug 1303708 Opened 4 years ago Closed 4 years ago

Remove unused JSID_IS_ZERO function.

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox51 --- fix-optional
firefox52 --- fixed

People

(Reporter: arai, Assigned: userajay0, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [good first bug] [lang=c++])

Attachments

(2 files, 1 obsolete file)

The function to be removed has been removed
5.73 KB, patch
Details | Diff | Splinter Review
Bug 1303708 - Remove unused JSID_IS_ZERO(jsid id) function.
58 bytes, text/x-review-board-request
arai
: review+
Details 
JSID_IS_ZERO function was added 5 years ago by bug 601457 to catch unexpected case, and it's not used anymore.
we could remove it now.
(if not, we would need to add a comment that explains the purpose of the function)

https://dxr.mozilla.org/mozilla-central/rev/f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc/js/public/Id.h#72
> static MOZ_ALWAYS_INLINE bool
> JSID_IS_ZERO(jsid id)
> {
>     return JSID_BITS(id) == 0;
> }
Hello ! 
I am new here , Can i request to get this bug assigned to me ?
Thank you for your comment :)
I'll assign bug when a patch is posted.

if you have any question related to building, testing, etc, feel free to ask here or in IRC #introduction channel.
Although i do understand what needs to be done , can you tell me how do i go about providing a patch ? accessing this bug and rectifying it . 
thanks for your help .
you can provide a patch by either
  * push your changeset to review repository
  * export your patch as a patch file and attach it to this bug

about whole workflow, these documents will help
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html
Hi! I'm new here and finding my way about.
Can you help my way around here.
Thank you :)
(In reply to Amal Santhosh from comment #5)
> Hi! I'm new here and finding my way about.
> Can you help my way around here.
> Thank you :)

pass2pawan already commented they're going to work on this.
I filed bug 1304167 if you're looking for C++ good-first-bug.
Attached patch Patch file (obsolete) — — Splinter Review 
Hello,
I am a first timer. Please review the patch and tell me if something needs to be changed.

Thanks!
Attachment #8793169 - Flags: review?(arai.unmht)
(In reply to vinayakagarwal6996 from comment #7)
> Created attachment 8793169 [details] [diff] [review]
> Patch file
> 
> Hello,
> I am a first timer. Please review the patch and tell me if something needs
> to be changed.
> 
> Thanks!

Please delete this. I'm sorry. Wrong bug. :(
Comment on attachment 8793169 [details] [diff] [review]
Patch file

per comment #8.
Attachment #8793169 - Attachment is obsolete: true
Attachment #8793169 - Flags: review?(arai.unmht)
Tooru , i have the build in mercurial , is there any comprehensive guide which i can follow to track down the code and debug and patch it ? or could you help me in anyway for doing that?
the file is in js/public/Id.h, under mozilla-central (or mozilla-unified)
about debugging and patching, what's your question?
I found the file and saved it in my build . now after this how do i create a patch ? what commands do i run on mercurial.
here's guide for workflow
  https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html

about creating patch, you need to create a changeset, by
  hg commit -m 'Bug 1303708 - Remove unused JSID_IS_ZERO function.'

then, push it to review repository after configuring this:
  https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html#mozreview-user

or, just export it and attach the file here:
  hg export -r . > PATH_TO_OUTPUT_PATCH_FILE.patch
(In reply to ajay.krishn97 from comment #14)
> Created attachment 8793493 [details] [diff] [review]
> The function to be removed has been removed

you need to export your change as a patch file:
  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Step_4_-_Get_your_code_reviewed
or submit it to review repository:
  https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html
Attachment #8793493 - Flags: review?(arai.unmht)
Comment on attachment 8793493 [details] [diff] [review]
The function to be removed has been removed

please read comment #15 ;)
Attachment #8793493 - Flags: review?(arai.unmht)
(In reply to Tooru Fujisawa [:arai] from comment #16)
> Comment on attachment 8793493 [details] [diff] [review]
> The function to be removed has been removed
> 
> please read comment #15 ;)

Is your Mozreview Username 'arai'?

Thanks
(In reply to ajay.krishn97 from comment #17)
> (In reply to Tooru Fujisawa [:arai] from comment #16)
> > Comment on attachment 8793493 [details] [diff] [review]
> > The function to be removed has been removed
> > 
> > please read comment #15 ;)
> 
> Is your Mozreview Username 'arai'?
> 
> Thanks

yes
After making the change I ran hg commit with following commit message 
              r?arai Bug 1303708
              Removed the function that had to be removed
Then I got the ouput 'created new head'
Then I ran hg push review and I get the message
               pushing to https://reviewboard-hg.mozilla.org/autoreview
               searching for appropriate review repository
               redirecting push to https://reviewboard-hg.mozilla.org/gecko
               abort: you must set mozilla.ircnick in your hgrc config file to your IRC nickname in order to perform code reviews

Could you help me solve this issue

Thanks
Comment on attachment 8793560 [details]
Bug 1303708 - Remove unused JSID_IS_ZERO(jsid id)  function.

https://reviewboard.mozilla.org/r/80276/#review78918

Thank you for your patch :)

the code change looks good.
Can you change the commit message to follow this rule?
  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Commit_message_restrictions

so, put bug number and summary in the same line, and also describe the change more specifically, like:
  Bug 1303708 - Remove unused JSID_IS_ZERO function.
or something similar.
Attachment #8793560 - Flags: review?(arai.unmht)
Assignee: nobody → ajay.krishn97
Status: NEW → ASSIGNED
pass2pawan, ping me if you're looking for other C++ bug.
Comment on attachment 8793560 [details]
Bug 1303708 - Remove unused JSID_IS_ZERO(jsid id)  function.

https://reviewboard.mozilla.org/r/80276/#review78918

So do I have to change the commit message(through 'hg commit --amend') and 'hg push review' it again or can I change it elsewhere(for example on reviewboard.mozilla.org)
I'm sorry if the solution to my doubts are too obvious, I'm a new user here.

Thanks
Comment on attachment 8793560 [details]
Bug 1303708 - Remove unused JSID_IS_ZERO(jsid id)  function.

https://reviewboard.mozilla.org/r/80276/#review78918

`hg commit --amend` will work, I think.
Comment on attachment 8793560 [details]
Bug 1303708 - Remove unused JSID_IS_ZERO(jsid id)  function.

https://reviewboard.mozilla.org/r/80276/#review78928

Perfect :)
Attachment #8793560 - Flags: review?(arai.unmht) → review+
Comment on attachment 8793560 [details]
Bug 1303708 - Remove unused JSID_IS_ZERO(jsid id)  function.

https://reviewboard.mozilla.org/r/80276/#review78928

Could you tell me what I have to do next?

Thanks
I pushed your patch to try server.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=14ccea76c706

after it successfully finishes, the patch can be landed.
if you don't have a permission to do so (I'm not sure who can/cannot, on MozReview), ping me.
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/autoland/rev/ef5324ac2924
Remove unused JSID_IS_ZERO(jsid id)  function. r=arai
Comment on attachment 8793560 [details]
Bug 1303708 - Remove unused JSID_IS_ZERO(jsid id)  function.

https://reviewboard.mozilla.org/r/80276/#review78928

Is there anything still left to do with regard to this bug or is the whole process finished
almost done.
just wait for the patch be merged to mozilla-central.

if something goes wrong (like it causes some issue), the patch will be backed out and you'll need to fix it,
if nothing goes wrong, it's finished :)
https://hg.mozilla.org/mozilla-central/rev/ef5324ac2924
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(In reply to Tooru Fujisawa [:arai] from comment #32)
> almost done.
> just wait for the patch be merged to mozilla-central.
> 
> if something goes wrong (like it causes some issue), the patch will be
> backed out and you'll need to fix it,
> if nothing goes wrong, it's finished :)

So everything regarding resolving this bug is finished right?
 Thank you for all the help
Yes, finished.
Thank you again for your contribution :)
Mark 51 as fix-optional. If it's worth uplift to 51, feel free to nominate it.
status-firefox51: affected → fix-optional
You need to log in before you can comment on or make changes to this bug.