Closed Bug 1464321 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: arai, Assigned: maharsh312, Mentored)

Details

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

Attachments

(1 file, 1 obsolete file)

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.
Can you assign me this bug?

I'll like to work on it.
Assignee: nobody → maharsh312
Status: NEW → ASSIGNED
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+
I'll file the bug for it and complete the patch.
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+
Getting 4 errors, how do i solve them?
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.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/2e7a4d376aff
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.