816805 - Style fixes for RiverTrail code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: rename files

This patch fixes the filename issues. Niko is going to work on the remaining formatting issues.

Comment 1

[Shu-yu Guo [:shu]]

Comment on attachment 686887 [details] [diff] [review]
rename files

Stealing the review.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a12647c876

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Style fixes

Actual style fixes for the files.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 686901 [details] [diff] [review]
Style fixes

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

Given some of the comments I've made, I'm sure there's more that should be done on the style-fixing front, but hey, let's take forward progress when we can.  More fixing is good, needn't hold this up just to get it done, tho.

::: js/src/vm/ForkJoin.cpp
@@ +25,2 @@
>      : public TaskExecutor,
>        public Monitor

Indentation for this should be:

class js::ForkJoinShared
  : public TaskExecutor,
    public Monitor

Although this is short enough it's probably best all on one line:

class js::ForkJoinShared : public TaskExecutor, public Monitor

@@ +124,5 @@
>      JSRuntime *runtime() { return cx_->runtime; }
>  };
>  
> +class js::AutoRendezvous {
> +  private:

{ on new line.

@@ +213,5 @@
>  ForkJoinShared::~ForkJoinShared()
>  {
>      PR_DestroyCondVar(rendezvousEnd_);
>  
> +    while (arenaListss_.length() > 0)

Erm, is arenaListss_ a typo?  rs=me to fix in a separate change if it is (I'm not checking, but it's hard to imagine a meaning where it's not a typo :-) ).

@@ +312,5 @@
>  bool
>  ForkJoinShared::setFatal()
>  {
> +    // Might as well set the abort flag to true, it will make propagation
> +    // faster:

Sentence fragment-ish thing ending in a colon?  Probably should make this a complete sentence, as I can't remember ever seeing anything like this in SpiderMonkey before.

@@ +499,2 @@
>      return TP_RETRY_SEQUENTIALLY;
> +#else

It's probably better style for this not to check for non-definition -- it's easier to read positive assertions than negative ones.

::: js/src/vm/ForkJoin.h
@@ +23,5 @@
> +// > class MyForkJoinOp {
> +// >   ... define callbacks as appropriate for your operation ...
> +// > };
> +// > MyForkJoinOp op;
> +// > ExecuteForkJoinOp(cx, op);

It's nitpicky, but we'd just use the indentation and block-formatting to set this off, no >.

@@ +75,5 @@
> +//   |parallel()| callback of a |ForkJoinOp| may not itself invoke
> +//   |ExecuteForkJoinOp()|.  We may lift this limitation in the future.
> +//
> +// - No load balancing is performed between worker threads.  That means that
> +//   the fork-join system is best suited for problems that can be slice into

sliced

(no, I'm not actually reading all these comments, this just happened to catch an eye)

@@ +178,5 @@
>      // Returns true on success, false to halt parallel execution.
>      virtual bool parallel(ForkJoinSlice &slice) = 0;
>  
>      // Invoked after parallel phase ends if execution was successful
> +    // (not aborted)q

q

::: js/src/vm/Monitor.cpp
@@ +11,4 @@
>  
>  Monitor::Monitor()
>      : lock_(NULL), condVar_(NULL)
> +{}

Any reason this isn't defined inline in the header?  Not that it matters much perf-wise, but no reason to leave it on the table.

@@ +16,5 @@
>  Monitor::~Monitor()
>  {
>  #ifdef JS_THREADSAFE
>      PR_DestroyLock(lock_);
>      PR_DestroyCondVar(condVar_);

Um...shouldn't these be conditioned on those pointers being non-null?

::: js/src/vm/ThreadPool.cpp
@@ +26,2 @@
>  
>  #define WORKER_THREAD_STACK_SIZE (1*1024*1024)

This should be const size_t.

@@ +66,5 @@
>      void terminate();
>  };
>  
>  ThreadPoolWorker::ThreadPoolWorker(size_t workerId, ThreadPool *tp)
>      : workerId_(workerId), threadPool_(tp), state_(CREATED), worklist_()

This should be indented two spaces.

@@ +173,3 @@
>          while (state_ != TERMINATED) {
>              lock.wait();
>          }

SpiderMonkey doesn't brace single-line bodies, when the condition (here, |state_ != TERMINATED|) and any other arms (if you have an if-else if-else) each fit on single lines.

@@ +174,5 @@
>              lock.wait();
>          }
>      } else {
>          JS_ASSERT(state_ == TERMINATED);
>      }

For example, this else-body is a single line, but because the if-body above is more than one line, we brace this, as you've correctly done.

@@ +186,4 @@
>  
>  ThreadPool::ThreadPool(JSRuntime *rt)
>      : runtime_(rt),
>        nextId_(0)

Two spaces.

@@ +238,5 @@
>  ThreadPool::terminateWorkers()
>  {
>      for (size_t i = 0; i < workers_.length(); i++) {
>          workers_[i]->terminate();
>      }

Nor should this be braced.

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a1a12647c876

Comment 6

[Jonathan Kew [:jfkthame]]

The case-changing of filenames here is going to cause Mercurial grief for people on case-insensitive filesystems, I think.

Comment 7

[Shu-yu Guo [:shu]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)

> @@ +213,5 @@
> >  ForkJoinShared::~ForkJoinShared()
> >  {
> >      PR_DestroyCondVar(rendezvousEnd_);
> >  
> > +    while (arenaListss_.length() > 0)
> 
> Erm, is arenaListss_ a typo?  rs=me to fix in a separate change if it is
> (I'm not checking, but it's hard to imagine a meaning where it's not a typo
> :-) ).
> 

It's not a typo, Niko has said it's for "lists of lists", as the data structure for arena lists is already plural as ArenaLists.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Jonathan Kew (:jfkthame) from comment #6)
> The case-changing of filenames here is going to cause Mercurial grief for
> people on case-insensitive filesystems, I think.

How so?

Comment 9

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e418cb281a3a

https://tbpl.mozilla.org/?tree=Try&rev=8b8db13a23d6 without tests, since these are just style fixes.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e418cb281a3a