Last Comment Bug 673499 - Remove Whitespace
: Remove Whitespace
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Tom Schuster [:evilpie]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-22 11:51 PDT by Tom Schuster [:evilpie]
Modified: 2012-05-19 18:37 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
remove whitespace (2.54 MB, patch)
2011-07-22 11:52 PDT, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
remove trailing whitespace (2.42 MB, patch)
2011-07-22 13:30 PDT, Tom Schuster [:evilpie]
jorendorff: review-
Details | Diff | Splinter Review
smaller patch (177.71 KB, patch)
2012-05-16 08:53 PDT, Tom Schuster [:evilpie]
jorendorff: review+
Details | Diff | Splinter Review

Description Tom Schuster [:evilpie] 2011-07-22 11:51:19 PDT

    
Comment 1 Tom Schuster [:evilpie] 2011-07-22 11:52:15 PDT
Created attachment 547762 [details] [diff] [review]
remove whitespace
Comment 2 Tom Schuster [:evilpie] 2011-07-22 13:30:59 PDT
Created attachment 547792 [details] [diff] [review]
remove trailing whitespace
Comment 3 :Ms2ger 2011-07-22 14:01:23 PDT
Comment on attachment 547792 [details] [diff] [review]
remove trailing whitespace

Leave ctypes/libffi.patch and ctypes/libffi/* alone. If you're going to touch /js/src/configure.in, check if you need to update /configure.in as well. Maybe leave xpconnect/ alone as well, and yarr/ if we update it from upstream.

r=me with those changes, but keep in mind I'm technically not qualified to review stuff.
Comment 4 Jason Orendorff [:jorendorff] 2011-07-25 12:41:28 PDT
Comment on attachment 547792 [details] [diff] [review]
remove trailing whitespace

This patch is too broad.

I'd review a patch restricted to C/C++ files in js/src and the subdirectories js/src/{vm,methodjit,tracejit,shell}.

I'd also be willing to review a separate patch restricted to js/src/{jit-test,tests,jsapi-tests}, but it's maybe not such a bad thing to have a few tests that contain trailing whitespace; some of those may even be intentional.

Much of the rest is code other teams are responsible for, sometimes copied from upstream sources. We should not make unnecessary changes there.
Comment 5 Ed Morley [:emorley] 2011-08-04 18:00:07 PDT
If it helps at all, the js/src/ build system whitespace cleanup will be dealt with by Matheus' patch in bug 671465.
Comment 6 Tom Schuster [:evilpie] 2012-05-16 08:53:59 PDT
Created attachment 624398 [details] [diff] [review]
smaller patch
Comment 7 Jason Orendorff [:jorendorff] 2012-05-17 13:02:11 PDT
Comment on attachment 624398 [details] [diff] [review]
smaller patch

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

r=me with one comment.

::: js/src/ETWProvider.man
@@ +23,5 @@
>            <event symbol="EvtExecuteDone" value="1002" version="1"
>                   channel="MozillaChannel" level="win:Informational"
>                   template="CodeLocationTemplate" task="Execution" opcode="ExecuteStop"
>                   keywords="SampleKeyword" message="$(string.MozillaSpiderMonkey.ExecuteDone.message)"/>
> +

I have no idea what this file is. Surely this is autogenerated by some Microsoft tool that puts in the useless spaces, and the spaces will only come back the next time this is updated.

In any case you don't want your name on this file's changelog. Please revert this one.
Comment 8 Tom Schuster [:evilpie] 2012-05-19 13:06:16 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/c80dde3faa3d
And reverting the change to js/src/ETWProvider.man, which I accidentally did not remove after some rebasing action.
http://hg.mozilla.org/integration/mozilla-inbound/rev/71889cff519c
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-05-19 18:36:55 PDT
https://hg.mozilla.org/mozilla-central/rev/c80dde3faa3d
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-05-19 18:37:28 PDT
And follow-up:
https://hg.mozilla.org/mozilla-central/rev/71889cff519c

Note You need to log in before you can comment on or make changes to this bug.