1082679 - Rename JSTrapStatus

Status: RESOLVED DUPLICATE of bug 1443583

(Core :: JavaScript Engine, defect)

Comment 1

[abhinavdv]

I am a newbie . I would like to work on this bug . I have read the comments section in the bug1069694 but I could not figure out what JSTrapStatus should be renamed as?? Can anyone please help me??

Comment 2

[Jan de Mooij [:jandem]]

It would be nice to (1) change it to an enum class (2) rename to TrapStatus (3) move into the |js| namespace, something like this:

enum class TrapStatus {
    Error,
    Continue,
    Return,
    Throw,
    Limit
};

Thanks for working on this!

Comment 3

[abhinavdv]

Attachment: bug-1082679-v1.patch

Comment 4

[abhinavdv]

Can anyone please review the patch?

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8944241 [details] [diff] [review]
bug-1082679-v1.patch

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

Hi, thanks for the patch!

Did you compile the browser or JS shell with these changes? I'm asking because this patch changes Debugger.h, but JSTrapStatus is used in a lot of other files: https://searchfox.org/mozilla-central/search?q=symbol:T_JSTrapStatus&redirect=false

Let me know if you need help with that.

::: js/src/vm/Debugger.h
@@ +43,5 @@
> +    ERROR,
> +    CONTINUE,
> +    RETURN,
> +    THROW,
> +    LIMIT

It would be nice to rename these values to Error, Continue, Return, Throw, Limit

@@ +548,1 @@
>                                           MutableHandleValue vp,

We should also fix the "alignment" here and elsewhere.

Comment 6

[abhinavdv]

Thanks for reviewing the patch. 
I do not know to compile a patch. Is it just rebuilding Mozilla-central?
I will rename the values you talked about in the next patch.
Do I have to change jsTrapStatus everywhere by going into the code again and again or is there any other way.
I know little but I would love to learn. Can you please help me?
Thank you.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to [e-mail] from comment #6)
> I do not know to compile a patch. Is it just rebuilding Mozilla-central?

Yeah. This page might be useful for building the JS shell: https://wiki.mozilla.org/JavaScript:New_to_SpiderMonkey

> Do I have to change jsTrapStatus everywhere by going into the code again and
> again or is there any other way.

You could use sed or something: https://stackoverflow.com/questions/9704020/recursive-search-and-replace-in-text-files-on-mac-and-linux 

Some text editors also let you search-and-replace in all files in a directory.

Comment 8

[abhinavdv]

Attachment: bug-1082679.patch

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8946260 [details] [diff] [review]
bug-1082679.patch

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

::: js/src/jit/VMFunctions.cpp
@@ +1094,4 @@
>      MOZ_ASSERT(script->stepModeEnabled() || script->hasBreakpointsAt(pc));
>  
>      RootedValue rval(cx);
> +    TrapStatus status = JSTRAP_CONTINUE;

You renamed JSTRAP_CONTINUE so this won't compile.

Please make sure you can build the JS shell or Firefox with these changes - see the link I gave in comment 7. Let me know if you need help with that.

Comment 10

[abhinavdv]

Ok I will change that. I will recompile JS shell before uploading the next patch.

But, I have a doubt. If I push a new patch, The patch will contain only the new changes. But what about the other changes that are not there in the tip i.e, the changes made in the earlier patch?

Comment 11

[Jan de Mooij [:jandem]]

(In reply to [e-mail] from comment #10)
> But, I have a doubt. If I push a new patch, The patch will contain only the
> new changes. But what about the other changes that are not there in the tip
> i.e, the changes made in the earlier patch?

Do you use Mercurial or Git? If you use Mercurial with mq you can use qref to add changes to a patch or qfold to merge patches. Git also lets you squash patches.

Comment 12

[abhinavdv]

I use mercurial with hg

Comment 13

[Jan de Mooij [:jandem]]

Jason has a patch for this in bug 1443583.

Comment 14

[Jason Orendorff [:jorendorff]]

Oops. Sorry, abhinavdv!