Use same type for JSScript::{lineno,column} and LazyScript::{lineno,column}

RESOLVED FIXED in Firefox 62

Status

()

--
minor
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: arai, Assigned: maharsh312, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla62
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 months ago
JSScript and LazyScript provides almost same methods for some properties,
but the return type of lineno and column is different:

> class JSScript : public js::gc::TenuredCell
> {
> ...
>     size_t lineno() const {
>         return lineno_;
>     }
> 
>     size_t column() const {
>         return column_;
>     }

> class LazyScript : public gc::TenuredCell
> {
> ...
>     uint32_t lineno() const {
>         return lineno_;
>     }
>     uint32_t column() const {
>         return column_;
>     }

it's better using the same signature.

I guess uint32_t is fine for both, but haven't yet confirmed.
(Assignee)

Comment 1

10 months ago
Can you assign me this bug?

I'll like to work on it.
(Reporter)

Updated

10 months ago
Assignee: nobody → maharsh312
Status: NEW → ASSIGNED
(Reporter)

Comment 3

10 months ago
Comment on attachment 8980852 [details] [diff] [review]
Bug 1464321 - Changed return type of two methods in JSScript with relative format string change

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

Thank you for your patch :D
The change looks good.
There's one more place that uses JSScript::lineno that needs change,

https://searchfox.org/mozilla-central/rev/ce86c6c0472d5021ef693cf99abaaa0644c89e55/js/src/vm/Debugger.cpp#5420
> struct DebuggerScriptGetStartLineMatcher
> {
>     using ReturnType = uint32_t;
> 
>     ReturnType match(HandleScript script) {
>         return uint32_t(script->lineno());
>     }

The cast to uint32_t here can be removed now.


https://searchfox.org/mozilla-central/rev/ce86c6c0472d5021ef693cf99abaaa0644c89e55/js/src/vm/JSScript.h#1306
>     void setColumn(size_t column) { column_ = column; }

not related to this bug tho, given that the type of JSScript.column_ field is uint32_t,
it would be better using uint32_t as parameter type here.
can you file a new bug for it?
Attachment #8980852 - Flags: review?(arai.unmht) → feedback+
(Assignee)

Comment 4

10 months ago
I'll file the bug for it and complete the patch.
(Reporter)

Comment 6

10 months ago
Comment on attachment 8980915 [details] [diff] [review]
Bug 1464321 - Changed return type of two methods in JSScript with relative format string change (updated)

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

Thank you again!

here's try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=67220ddf39d067a2b7479e662bac1e0825cff511
this time I triggered a bit more automated tests, since the change is wide-spread.
it will take ~2 hours.
Attachment #8980915 - Flags: review?(arai.unmht) → review+
(Assignee)

Comment 7

10 months ago
Getting 4 errors, how do i solve them?
(Reporter)

Comment 8

10 months ago
they should be fine.
you can set checkin-needed.

when you click those failed jobs, you can see the failure summary in the bottom part of the page
if there's link to known intermittent failures, and the error message matches to it, you can ignore them.
for this case, there are matching known failures.
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 9

10 months ago
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e7a4d376aff
Changed return type of two methods in JSScript with relative format string change. r=arai
Keywords: checkin-needed

Comment 10

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2e7a4d376aff
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.