Closed Bug 1333020 Opened 3 years ago Closed Last year

Wasm baseline: Move some whitespace around in expressions

Categories

(Core :: JavaScript Engine: JIT, defect, P5)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: lth, Unassigned, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug)

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
(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.
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
I am new in this open source world, can I take up this bug?
Flags: needinfo?(lhansen)
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)
May I also work on this bug? Or is it only to be assigned to one person?
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.
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?
Is this issue still available? I'd like to work on this.
Flags: needinfo?(lhansen)
Yes, the bug is unassigned and I don't know about anyone actually working on it.
Flags: needinfo?(lhansen)
Where can I find the files I've to work on? Please guide me further.
Hi insiyeah, see comment 4 for more guidance.
I am a new beginner to open source world, can I also try to work on it?
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.
Resolved by bug 1447261.
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.