Mojo: mojob.sh: Make --release (etc.) work properly again.

Mojo: mojob.sh: Make --release (etc.) work properly again.

R=jamesr@chromium.org

Committed: https://crrev.com/3eadf8a6fea229000ed4978710789b23318ad2e9
Cr-Commit-Position: refs/heads/master@{#300408}

[jamesr, 2014-10-21 00:46:35]

https://codereview.chromium.org/663023006/diff/1/mojo/tools/mojob.sh
File mojo/tools/mojob.sh (right):

https://codereview.chromium.org/663023006/diff/1/mojo/tools/mojob.sh#newcode108
mojo/tools/mojob.sh:108: Debug)
i would leave this out - is_debug=true is the default, no need to pollute args

[viettrungluu, 2014-10-21 01:04:22]

https://codereview.chromium.org/663023006/diff/1/mojo/tools/mojob.sh
File mojo/tools/mojob.sh (right):

https://codereview.chromium.org/663023006/diff/1/mojo/tools/mojob.sh#newcode108
mojo/tools/mojob.sh:108: Debug)
On 2014/10/21 00:46:35, jamesr wrote:
> i would leave this out - is_debug=true is the default, no need to pollute args

My thinking is that if you make an explicit choice (e.g., debug vs release),
then you should put in the arg, since defaults are subject to change (and this
would silently start to do the wrong thing).

(An alternative is to compare the value vs the default and set it only if it's
not the default, but that's a lot of work.)

[jamesr, 2014-10-21 01:05:19]

I think it's valid to rely on the default.  We can't change it.

[viettrungluu, 2014-10-21 01:17:08]

On 2014/10/21 01:05:19, jamesr wrote:
> I think it's valid to rely on the default.  We can't change it.

I'm unconvinced (e.g., at some point we changed the default for clang), but OK.
PTAL.

[jamesr, 2014-10-21 01:21:11]

https://codereview.chromium.org/663023006/diff/20001/mojo/tools/mojob.sh
File mojo/tools/mojob.sh (right):

https://codereview.chromium.org/663023006/diff/20001/mojo/tools/mojob.sh#newc...
mojo/tools/mojob.sh:112: args+=("is_debug=false")
just need an 'if' here and not a case, right?

https://codereview.chromium.org/663023006/diff/20001/mojo/tools/mojob.sh#newc...
mojo/tools/mojob.sh:117: # (Default.)
we can change the default of clang - my comment was *only* about is_debug, which
I dont' think we will ever be able to change in the GN configuration

https://codereview.chromium.org/663023006/diff/20001/mojo/tools/mojob.sh#newc...
mojo/tools/mojob.sh:129: else
can just get rid of this branch, no?

https://codereview.chromium.org/663023006/diff/20001/mojo/tools/mojob.sh#newc...
mojo/tools/mojob.sh:133: disabled)
and get rid of this and simplify the 'case' to just an if ?

[viettrungluu, 2014-10-21 01:26:49]

On 2014/10/21 01:21:11, jamesr wrote:
> https://codereview.chromium.org/663023006/diff/20001/mojo/tools/mojob.sh
> File mojo/tools/mojob.sh (right):
> 
>
https://codereview.chromium.org/663023006/diff/20001/mojo/tools/mojob.sh#newc...
> mojo/tools/mojob.sh:112: args+=("is_debug=false")
> just need an 'if' here and not a case, right?

Well, yes, but a) I prefer to be mindless and consistent, and b) document the
possible valid values (it's easier to verify for correctness).

> 
>
https://codereview.chromium.org/663023006/diff/20001/mojo/tools/mojob.sh#newc...
> mojo/tools/mojob.sh:117: # (Default.)
> we can change the default of clang - my comment was *only* about is_debug,
which
> I dont' think we will ever be able to change in the GN configuration

But that means that we have to think about what might be changeable and what
might not!

> 
>
https://codereview.chromium.org/663023006/diff/20001/mojo/tools/mojob.sh#newc...
> mojo/tools/mojob.sh:129: else
> can just get rid of this branch, no?

Well, obviously, but it leaves the reader wondering if I didn't just forget
about the other case.

> 
>
https://codereview.chromium.org/663023006/diff/20001/mojo/tools/mojob.sh#newc...
> mojo/tools/mojob.sh:133: disabled)
> and get rid of this and simplify the 'case' to just an if ?

...

[jamesr, 2014-10-21 01:52:04]

Well if you wanted this to be maintainable long-term probably shouldn't have
written it in shell.

If you feel better being explicit about all the args then lgtm, but seems
wasteful to me.

[viettrungluu, 2014-10-21 02:31:02]

The CQ bit was checked by viettrungluu@chromium.org

[commit-bot: I haz the power, 2014-10-21 02:33:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/663023006/60001

[commit-bot: I haz the power, 2014-10-21 03:13:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-21 03:14:28]

Patchset 4 (id:??) landed as
https://crrev.com/3eadf8a6fea229000ed4978710789b23318ad2e9
Cr-Commit-Position: refs/heads/master@{#300408}