cleanup main

cleanup main

seems like these two lines are common to both paths and in fact
the different classes allocated are derived from the same common
base class so this makes sense to me.

BUG=
R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=43e8ab38ac6e7b865bbbc7a986fac24c53bace04

[rkotlerimgtec, 2015-12-02 22:14:24]

Description was changed from

==========
cleanup main

seems like these two lines are common to both paths and in fact
the different classes allocated are derived from the same common
base class so this makes sense to me.

BUG=
==========

to

==========
cleanup main

seems like these two lines are common to both paths and in fact
the different classes allocated are derived from the same common
base class so this makes sense to me.

BUG=
==========

[rkotlerimgtec, 2015-12-02 22:14:25]

rkotlerimgtec@gmail.com changed reviewers:
+ jpp@chromium.org, kschimpf@chromium.org, stichnot@chromium.org

[Jim Stichnoth, 2015-12-04 18:09:05]

https://codereview.chromium.org/1494753003/diff/20001/src/main.cpp
File src/main.cpp (right):

https://codereview.chromium.org/1494753003/diff/20001/src/main.cpp#newcode30
src/main.cpp:30: #else  // !PNACL_BROWSER_TRANSLATOR
We've generally tried to avoid conditional (#if) compilation as much as
possible, via the constexpr functions in Ice::BuildDefs.  It looks like this
instance may have slipped through.

Can you try adding a function like Ice::BuildDefs::browser() that returns
PNACL_BROWSER_TRANSLATOR, and use that to guard the code here?

[rkotlerimgtec, 2015-12-05 04:21:13]

Patchset #3 (id:40001) has been deleted

[rkotlerimgtec, 2015-12-05 04:30:19]

Patchset #3 (id:60001) has been deleted

[rkotlerimgtec, 2015-12-05 04:30:42]

Patchset #3 (id:80001) has been deleted

[rkotlerimgtec, 2015-12-05 05:20:05]

Patchset #3 (id:100001) has been deleted

[rkotlerimgtec, 2015-12-05 05:28:06]

Patchset #3 (id:120001) has been deleted

[rkotlerimgtec, 2015-12-05 05:42:58]

One question I had is whether we need to have compiler as a parameter to the
constructor for CompileServer. It seems like that is not needed and could just
be created in CompileServer since Compiler has no parameters for its
constructor.

That would simplify things some more but could also be done in a follow-on
patch.

[rkotlerimgtec, 2015-12-05 06:55:48]

ok. i'll add that to the patch this weekend.

On Fri, Dec 4, 2015 at 10:00 PM, <rkotlerimgtec@gmail.com> wrote:

> https://codereview.chromium.org/1494753003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[rkotlerimgtec, 2015-12-06 01:01:37]

Removed the compiler parameter from several constructors related to
CompilerServer becauase it's not needed.

[Jim Stichnoth, 2015-12-07 17:37:13]

https://codereview.chromium.org/1494753003/diff/170001/src/IceBrowserCompileS...
File src/IceBrowserCompileServer.h (right):

https://codereview.chromium.org/1494753003/diff/170001/src/IceBrowserCompileS...
src/IceBrowserCompileServer.h:45: explicit BrowserCompileServer()
Remove "explicit" for a zero-arg ctor.

https://codereview.chromium.org/1494753003/diff/170001/src/IceBrowserCompileS...
src/IceBrowserCompileServer.h:46: : InputStream(nullptr), HadError(false) {}
You can omit InputStream(nullptr) because of the "= nullptr" specification in
the declaration below.

https://codereview.chromium.org/1494753003/diff/170001/src/IceBuildDefs.h
File src/IceBuildDefs.h (right):

https://codereview.chromium.org/1494753003/diff/170001/src/IceBuildDefs.h#new...
src/IceBuildDefs.h:38: // PNACL_BROWSER_TRANSLATOR
Expand this comment like above and below.  E.g.:

PNACL_BROWSER_TRANSLATOR can be undefined, or defined to something non-zero to
indicate a browser-based translator.

https://codereview.chromium.org/1494753003/diff/170001/src/IceCompileServer.h
File src/IceCompileServer.h (right):

https://codereview.chromium.org/1494753003/diff/170001/src/IceCompileServer.h...
src/IceCompileServer.h:48: explicit CompileServer() {}
You don't need "explicit" for a zero-arg ctor.  Maybe something like:

  CompileServer() = default;

https://codereview.chromium.org/1494753003/diff/170001/src/IceCompileServer.h...
src/IceCompileServer.h:76: CLCompileServer(int argc, char **argv)
Be sure to run "make format", this should fit on one line.

https://codereview.chromium.org/1494753003/diff/170001/src/main.cpp
File src/main.cpp (right):

https://codereview.chromium.org/1494753003/diff/170001/src/main.cpp#newcode27
src/main.cpp:27: return Ice::BrowserCompileServer().runit();
Bikeshed: Instead of "runit", how about something more descriptive like
"runAndReturnErrorCode" ?

[rkotlerimgtec, 2015-12-07 18:21:12]

Patchset #6 (id:190001) has been deleted

[rkotlerimgtec, 2015-12-07 18:32:07]

https://codereview.chromium.org/1494753003/diff/170001/src/IceBrowserCompileS...
File src/IceBrowserCompileServer.h (right):

https://codereview.chromium.org/1494753003/diff/170001/src/IceBrowserCompileS...
src/IceBrowserCompileServer.h:45: explicit BrowserCompileServer()
On 2015/12/07 17:37:13, stichnot wrote:
> Remove "explicit" for a zero-arg ctor.

Done.

https://codereview.chromium.org/1494753003/diff/170001/src/IceBrowserCompileS...
src/IceBrowserCompileServer.h:46: : InputStream(nullptr), HadError(false) {}
On 2015/12/07 17:37:12, stichnot wrote:
> You can omit InputStream(nullptr) because of the "= nullptr" specification in
> the declaration below.

Done.

https://codereview.chromium.org/1494753003/diff/170001/src/IceBuildDefs.h
File src/IceBuildDefs.h (right):

https://codereview.chromium.org/1494753003/diff/170001/src/IceBuildDefs.h#new...
src/IceBuildDefs.h:38: // PNACL_BROWSER_TRANSLATOR
On 2015/12/07 17:37:13, stichnot wrote:
> Expand this comment like above and below.  E.g.:
> 
> PNACL_BROWSER_TRANSLATOR can be undefined, or defined to something non-zero to
> indicate a browser-based translator.

Done.

https://codereview.chromium.org/1494753003/diff/170001/src/IceCompileServer.h
File src/IceCompileServer.h (right):

https://codereview.chromium.org/1494753003/diff/170001/src/IceCompileServer.h...
src/IceCompileServer.h:48: explicit CompileServer() {}
On 2015/12/07 17:37:13, stichnot wrote:
> You don't need "explicit" for a zero-arg ctor.  Maybe something like:
> 
>   CompileServer() = default;

Done.

https://codereview.chromium.org/1494753003/diff/170001/src/IceCompileServer.h...
src/IceCompileServer.h:76: CLCompileServer(int argc, char **argv)
On 2015/12/07 17:37:13, stichnot wrote:
> Be sure to run "make format", this should fit on one line.

Done.

https://codereview.chromium.org/1494753003/diff/170001/src/main.cpp
File src/main.cpp (right):

https://codereview.chromium.org/1494753003/diff/170001/src/main.cpp#newcode27
src/main.cpp:27: return Ice::BrowserCompileServer().runit();
On 2015/12/07 17:37:13, stichnot wrote:
> Bikeshed: Instead of "runit", how about something more descriptive like
> "runAndReturnErrorCode" ?

Done.

[Jim Stichnoth, 2015-12-07 21:00:55]

https://codereview.chromium.org/1494753003/diff/210001/src/main.cpp
File src/main.cpp (right):

https://codereview.chromium.org/1494753003/diff/210001/src/main.cpp#newcode29
src/main.cpp:29: return Ice::CLCompileServer(argc,
argv).runAndReturnErrorCode();
Use symmetric braces for if/else, i.e.

  if (a)
    x;
  else
    y;

or

  if (a) {
    x;
  } else {
    y;
  }

but not

  if (a) {
    x;
  } else
    y;

[rkotlerimgtec, 2015-12-07 21:15:04]

I realized that we don't even need an else clause.

At least for LLVM rules, we would not even be allowed to

have an else clause following a return.

https://codereview.chromium.org/1494753003/diff/210001/src/main.cpp
File src/main.cpp (right):

https://codereview.chromium.org/1494753003/diff/210001/src/main.cpp#newcode29
src/main.cpp:29: return Ice::CLCompileServer(argc,
argv).runAndReturnErrorCode();
On 2015/12/07 21:00:55, stichnot wrote:
> Use symmetric braces for if/else, i.e.
> 
>   if (a)
>     x;
>   else
>     y;
> 
> or
> 
>   if (a) {
>     x;
>   } else {
>     y;
>   }
> 
> but not
> 
>   if (a) {
>     x;
>   } else
>     y;

Done.

[Jim Stichnoth, 2015-12-07 21:51:49]

lgtm

[Jim Stichnoth, 2015-12-07 22:31:03]

Description was changed from

==========
cleanup main

seems like these two lines are common to both paths and in fact
the different classes allocated are derived from the same common
base class so this makes sense to me.

BUG=
==========

to

==========
cleanup main

seems like these two lines are common to both paths and in fact
the different classes allocated are derived from the same common
base class so this makes sense to me.

BUG=
R=stichnot@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[Jim Stichnoth, 2015-12-07 22:31:05]

Committed patchset #7 (id:230001) manually as
43e8ab38ac6e7b865bbbc7a986fac24c53bace04 (presubmit successful).