Emit error if unknown switch is passed to mojo_shell

Emit error if unknown switch is passed

BUG=
R=aa@chromium.org, hansmuller@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/401d5073b636b80600e8cef4ff0a9e12062ece0c

[DaveMoore, 2014-11-16 23:48:50]

Remove extra import

[DaveMoore, 2014-11-16 23:49:28]

davemoore@chromium.org changed reviewers:
+ hansmuller@chromium.org

[hansmuller, 2014-11-17 15:33:19]

LGTM with some minor suggestions.

https://codereview.chromium.org/731923002/diff/20001/mojo/shell/desktop/mojo_...
File mojo/shell/desktop/mojo_main.cc (right):

https://codereview.chromium.org/731923002/diff/20001/mojo/shell/desktop/mojo_...
mojo/shell/desktop/mojo_main.cc:114: std::cerr << "unknown switch: " << s.first
<< std::endl;
It might be helpful if you included argv[0] in the error message.

https://codereview.chromium.org/731923002/diff/20001/mojo/shell/switches.cc
File mojo/shell/switches.cc (right):

https://codereview.chromium.org/731923002/diff/20001/mojo/shell/switches.cc#n...
mojo/shell/switches.cc:67: static std::set<std::string> switch_set;
The switch_array and the cached set values aren't performance critical. It might
be better to just make switch_array[] a local in GetAllSwitches() and create the
set each time the function is called.

[Aaron Boodman, 2014-11-17 15:46:06]

aa@chromium.org changed reviewers:
+ aa@chromium.org

[Aaron Boodman, 2014-11-17 15:46:07]

nlgtm

https://codereview.chromium.org/731923002/diff/20001/mojo/shell/switches.cc
File mojo/shell/switches.cc (right):

https://codereview.chromium.org/731923002/diff/20001/mojo/shell/switches.cc#n...
mojo/shell/switches.cc:55: kArgsFor,
This duplicates information. People are going to update the list of switches,
but forget to update this. In fact, it has already happened -- switch_array is
missing kContentHandlers.

I think if you want to make this change, you should find a way to avoid the
duplicate information. Somehow, defining a switch should add it to the list of
switches.

Sorry.

PS: naming-wise, it should be: kSwitchArray.

https://codereview.chromium.org/731923002/diff/20001/mojo/shell/switches.cc#n...
mojo/shell/switches.cc:67: static std::set<std::string> switch_set;
On 2014/11/17 15:33:19, hansmuller wrote:
> The switch_array and the cached set values aren't performance critical. It
might
> be better to just make switch_array[] a local in GetAllSwitches() and create
the
> set each time the function is called.

It's against Chromium style to have static non-POD types anyway. We spent a lot
of time in Chromium trying to find all of them and kill them because they hurt
startup perf.

[DaveMoore, 2014-11-17 16:22:47]

Address review issues

[DaveMoore, 2014-11-17 16:35:31]

Remove static

[DaveMoore, 2014-11-17 16:36:47]

https://codereview.chromium.org/731923002/diff/20001/mojo/shell/switches.cc
File mojo/shell/switches.cc (right):

https://codereview.chromium.org/731923002/diff/20001/mojo/shell/switches.cc#n...
mojo/shell/switches.cc:55: kArgsFor,
On 2014/11/17 15:46:06, Aaron Boodman wrote:
> This duplicates information. People are going to update the list of switches,
> but forget to update this. In fact, it has already happened -- switch_array is
> missing kContentHandlers.
> 
> I think if you want to make this change, you should find a way to avoid the
> duplicate information. Somehow, defining a switch should add it to the list of
> switches.
> 
> Sorry.
> 
> PS: naming-wise, it should be: kSwitchArray.

Name changed. I didn't think you used kFoo for arrays.

It seems pretty unlikely that you'll add a switch and not add it to this array,
as then your switch can't work. kContentHandlers wasn't like that. It was simply
a failure of the cl (thanks).

https://codereview.chromium.org/731923002/diff/20001/mojo/shell/switches.cc#n...
mojo/shell/switches.cc:67: static std::set<std::string> switch_set;
On 2014/11/17 15:46:07, Aaron Boodman wrote:
> On 2014/11/17 15:33:19, hansmuller wrote:
> > The switch_array and the cached set values aren't performance critical. It
> might
> > be better to just make switch_array[] a local in GetAllSwitches() and create
> the
> > set each time the function is called.
> 
> It's against Chromium style to have static non-POD types anyway. We spent a
lot
> of time in Chromium trying to find all of them and kill them because they hurt
> startup perf.

I see now that you're right that even function statics can't be classes. That
surprised me because they don't invoke static initializers so they have no
impact on startup. Perhaps that's compiler specific.

I changed it to return a new map each time. It's only really going to be called
once at startup anyway.

[Aaron Boodman, 2014-11-17 16:44:45]

https://codereview.chromium.org/731923002/diff/20001/mojo/shell/switches.cc
File mojo/shell/switches.cc (right):

https://codereview.chromium.org/731923002/diff/20001/mojo/shell/switches.cc#n...
mojo/shell/switches.cc:55: kArgsFor,
On 2014/11/17 16:36:47, DaveMoore wrote:
> On 2014/11/17 15:46:06, Aaron Boodman wrote:
> > This duplicates information. People are going to update the list of
switches,
> > but forget to update this. In fact, it has already happened -- switch_array
is
> > missing kContentHandlers.
> > 
> > I think if you want to make this change, you should find a way to avoid the
> > duplicate information. Somehow, defining a switch should add it to the list
of
> > switches.
> > 
> > Sorry.
> > 
> > PS: naming-wise, it should be: kSwitchArray.
> 
> Name changed. I didn't think you used kFoo for arrays.
> 
> It seems pretty unlikely that you'll add a switch and not add it to this
array,
> as then your switch can't work. kContentHandlers wasn't like that. It was
simply
> a failure of the cl (thanks).

Sorry, I disagree. I think it is likely that people will add a switch and not
know or remember this all_switches variable. Especially if they aren't in the
same screenful of code.

Perhaps there is some way you can register the flag right below the line it is
defined... But I can't think of how to do that without significantly refactoring
how switches or adding a static vector.

[DaveMoore, 2014-11-17 16:57:16]

On 2014/11/17 16:44:45, Aaron Boodman wrote:
> https://codereview.chromium.org/731923002/diff/20001/mojo/shell/switches.cc
> File mojo/shell/switches.cc (right):
> 
>
https://codereview.chromium.org/731923002/diff/20001/mojo/shell/switches.cc#n...
> mojo/shell/switches.cc:55: kArgsFor,
> On 2014/11/17 16:36:47, DaveMoore wrote:
> > On 2014/11/17 15:46:06, Aaron Boodman wrote:
> > > This duplicates information. People are going to update the list of
> switches,
> > > but forget to update this. In fact, it has already happened --
switch_array
> is
> > > missing kContentHandlers.
> > > 
> > > I think if you want to make this change, you should find a way to avoid
the
> > > duplicate information. Somehow, defining a switch should add it to the
list
> of
> > > switches.
> > > 
> > > Sorry.
> > > 
> > > PS: naming-wise, it should be: kSwitchArray.
> > 
> > Name changed. I didn't think you used kFoo for arrays.
> > 
> > It seems pretty unlikely that you'll add a switch and not add it to this
> array,
> > as then your switch can't work. kContentHandlers wasn't like that. It was
> simply
> > a failure of the cl (thanks).
> 
> Sorry, I disagree. I think it is likely that people will add a switch and not
> know or remember this all_switches variable. Especially if they aren't in the
> same screenful of code.
> 
> Perhaps there is some way you can register the flag right below the line it is
> defined... But I can't think of how to do that without significantly
refactoring
> how switches or adding a static vector.

You're saying that people will add a switch and never once try to use it?
Because the switch can't work if they don't add it to the array.

[Aaron Boodman, 2014-11-17 17:00:06]

On 2014/11/17 16:57:16, DaveMoore wrote:
> On 2014/11/17 16:44:45, Aaron Boodman wrote:
> > https://codereview.chromium.org/731923002/diff/20001/mojo/shell/switches.cc
> > File mojo/shell/switches.cc (right):
> > 
> >
>
https://codereview.chromium.org/731923002/diff/20001/mojo/shell/switches.cc#n...
> > mojo/shell/switches.cc:55: kArgsFor,
> > On 2014/11/17 16:36:47, DaveMoore wrote:
> > > On 2014/11/17 15:46:06, Aaron Boodman wrote:
> > > > This duplicates information. People are going to update the list of
> > switches,
> > > > but forget to update this. In fact, it has already happened --
> switch_array
> > is
> > > > missing kContentHandlers.
> > > > 
> > > > I think if you want to make this change, you should find a way to avoid
> the
> > > > duplicate information. Somehow, defining a switch should add it to the
> list
> > of
> > > > switches.
> > > > 
> > > > Sorry.
> > > > 
> > > > PS: naming-wise, it should be: kSwitchArray.
> > > 
> > > Name changed. I didn't think you used kFoo for arrays.
> > > 
> > > It seems pretty unlikely that you'll add a switch and not add it to this
> > array,
> > > as then your switch can't work. kContentHandlers wasn't like that. It was
> > simply
> > > a failure of the cl (thanks).
> > 
> > Sorry, I disagree. I think it is likely that people will add a switch and
not
> > know or remember this all_switches variable. Especially if they aren't in
the
> > same screenful of code.
> > 
> > Perhaps there is some way you can register the flag right below the line it
is
> > defined... But I can't think of how to do that without significantly
> refactoring
> > how switches or adding a static vector.
> 
> You're saying that people will add a switch and never once try to use it?
> Because the switch can't work if they don't add it to the array.

I didn't realize that. I still think it's questionable, but whatever.

lgtm.

[DaveMoore, 2014-11-17 17:01:41]

Committed patchset #4 (id:60001) manually as
401d5073b636b80600e8cef4ff0a9e12062ece0c (presubmit successful).

[eseidel, 2014-11-17 19:30:21]

eseidel@chromium.org changed reviewers:
+ eseidel@chromium.org

[eseidel, 2014-11-17 19:30:21]

I believe this broke skydb:
https://codereview.chromium.org/735463004/

--v=1 used to work at least...