Wasm baseline: Move some whitespace around in expressions

RESOLVED FIXED

Status

()

Core
JavaScript Engine: JIT
P5
normal
RESOLVED FIXED
a year ago
13 days ago

People

(Reporter: lth, Unassigned, Mentored)

Tracking

(Blocks: 1 bug, {good-first-bug})

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

a year ago
Some of us are more concerned than others about where we put our whitespace, especially in expressions and loop headers.

Since I belong to the 100%-unconcerned faction (by temperament), and since an idiosyncratic and heterodox use of whitespace is built into my fingers (by decades of programming), my code tends to annoy those on the more concerned + orthodox end of the spectrum.  Anyone who wants to go over the baseline compiler and clean up whitespace is welcome to do so but please, please, please -- make modest, meaningful changes, and submit them for review in modest chunks.
As part of the witch hunters faction, here are a few more comments:

- please do change no more than 5 or 6 functions at a time *in a single patch*, to not make rebasing painful for other programmers.
- an example of the required changes is the following: whenever we have `x+y`, we want to have `x + y`.
- other spacing changes could impact line sizes: per our guidelines, a code line may fill up to 100 columns, but a comment may fill up to 80 chars only, for some reason. There are a few comments that should be wrapped in the baseline compiler file (js/src/wasm/WasmBaselineCompile.cpp).

If you have any questions, feel free to reach us on irc (irc.mozilla.org, channel #ionmonkey or #jsapi; I'm bbouvier there).
Priority: -- → P5
(Reporter)

Updated

a year ago
Blocks: 1316801
(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> per our guidelines, a code
> line may fill up to 100 columns, but a comment may fill up to 80 chars only,
> for some reason.

Actually it's 99ch and 79ch, the off-by-one to accommodate line-continuation glyphs in Emacs, apparently.  I kinda think pandering to Emacs this way is dumb.  But also, whatevs.

As for why comments are less, it's to have less-long lines to aid readability and simplify the visual reset-to-text-start when reading paragraphs.

Comment 3

10 months ago
I seek guidance from mentors Benjamin and Lars to get started. I am interested to work on this.
(In reply to Abhishek Tripathi from comment #3)
> I seek guidance from mentors Benjamin and Lars to get started. I am
> interested to work on this.

Hi Abhishek, thanks for jumping in! See comment 1 for some guidance. If you have made all the following to start hacking on Spidermonkey [1], you can start hacking on the /js/src/wasm/WasmBaselineCompile.{h,cpp} files and change whitespaces there.

If you get stuck or need any help, feel free to drop by on IRC (irc.mozilla.org), in the #ionmonkey or #jsapi channels; my nick there is bbouvier.

[1] https://wiki.mozilla.org/JavaScript:New_to_SpiderMonkey

Comment 5

8 months ago
I am new in this open source world, can I take up this bug?

Updated

8 months ago
Flags: needinfo?(lhansen)
(Reporter)

Comment 6

8 months ago
Hi meghasharma4910, you can indeed work on this if you like, it's a good way to get started.  See comment 4 for more guidance, and do feel free to ask if you are stuck.
Flags: needinfo?(lhansen)

Comment 7

8 months ago
May I also work on this bug? Or is it only to be assigned to one person?

Comment 8

8 months ago
I've started working on the bug, though was facing some problems with the setup. I'll try to send the patch as soon as possible.
(Reporter)

Comment 9

8 months ago
Hi Gauri,

it's best if only one person works at a bug at a time, and especially in this case, as only one file is involved.  However it looks like bug 1333018 has been idle for a while, perhaps you want to take a look at that?

Comment 10

2 months ago
Is this issue still available? I'd like to work on this.
Flags: needinfo?(lhansen)
(Reporter)

Comment 11

2 months ago
Yes, the bug is unassigned and I don't know about anyone actually working on it.
Flags: needinfo?(lhansen)

Comment 12

2 months ago
Where can I find the files I've to work on? Please guide me further.
(Reporter)

Comment 13

a month ago
Hi insiyeah, see comment 4 for more guidance.

Comment 14

a month ago
I am a new beginner to open source world, can I also try to work on it?
(Reporter)

Comment 15

a month ago
Hi George, I propose you look at some other bug, I think this one is going to be resolved shortly with work that Benjamin is doing over in bug 1447261.
(Reporter)

Comment 16

13 days ago
Resolved by bug 1447261.
Status: NEW → RESOLVED
Last Resolved: 13 days ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.