Closed
Bug 1464321
Opened 6 years ago
Closed 6 years ago
Use same type for JSScript::{lineno,column} and LazyScript::{lineno,column}
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
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.
Assignee | ||
Comment 1•6 years ago
|
||
Can you assign me this bug? I'll like to work on it.
Reporter | ||
Updated•6 years ago
|
Assignee: nobody → maharsh312
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #8980852 -
Flags: review?(arai.unmht)
Reporter | ||
Comment 3•6 years 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•6 years ago
|
||
I'll file the bug for it and complete the patch.
Assignee | ||
Comment 5•6 years ago
|
||
Attachment #8980852 -
Attachment is obsolete: true
Attachment #8980915 -
Flags: review?(arai.unmht)
Reporter | ||
Comment 6•6 years 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•6 years ago
|
||
Getting 4 errors, how do i solve them?
Reporter | ||
Comment 8•6 years 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•6 years ago
|
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
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2e7a4d376aff
Status: ASSIGNED → RESOLVED
Closed: 6 years 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.
Description
•