mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gastón Kleiman <gas...@mesosphere.io>
Subject Re: Review Request 57560: Removed unnecessary curly braces wrapping case statements.
Date Tue, 14 Mar 2017 10:57:26 GMT


> On March 13, 2017, 8:28 p.m., Jiang Yan Xu wrote:
> > What's the criteria for deeming such braces necessary or not? Some switch cases
in authorizer.cpp are left with braces. These cases are long of course, but is this the critera
used and "how long/complex is too long/complex"?

The idea of the patch was to be consistent at least within the same file. I would also be
fine with adding braces to all the cases.

Only the following two switch cases still have with braces:

https://github.com/apache/mesos/blob/fa7f4090a80b352dbc8bb9c5ee5053970e817826/src/authorizer/local/authorizer.cpp#L416-L446
https://github.com/apache/mesos/blob/fa7f4090a80b352dbc8bb9c5ee5053970e817826/src/authorizer/local/authorizer.cpp#L837-L866

I left the braces not because the cases are too long/complex, but because new variables are
declared there, and the file doesn't compile without the braces:


```
../../src/authorizer/local/authorizer.cpp: In member function ‘virtual Try<bool> mesos::internal::LocalAuthorizerObjectApprover::approved(const
Option<mesos::ObjectApprover::Object>&) const’:
../../src/authorizer/local/authorizer.cpp:446:29: error: jump to case label [-fpermissive]
         case authorization::VIEW_EXECUTOR:
                             ^~~~~~~~~~~~~
../../src/authorizer/local/authorizer.cpp:422:26: note:   crosses initialization of ‘Option<std::__cxx11::basic_string<char>
> taskUser’
           Option<string> taskUser = None();
                          ^~~~~~~~
../../src/authorizer/local/authorizer.cpp:459:29: error: jump to case label [-fpermissive]
         case authorization::LAUNCH_NESTED_CONTAINER:
                             ^~~~~~~~~~~~~~~~~~~~~~~
../../src/authorizer/local/authorizer.cpp:422:26: note:   crosses initialization of ‘Option<std::__cxx11::basic_string<char>
> taskUser’
           Option<string> taskUser = None();
                          ^~~~~~~~
../../src/authorizer/local/authorizer.cpp:460:29: error: jump to case label [-fpermissive]
         case authorization::LAUNCH_NESTED_CONTAINER_SESSION:
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/authorizer/local/authorizer.cpp:422:26: note:   crosses initialization of ‘Option<std::__cxx11::basic_string<char>
> taskUser’
           Option<string> taskUser = None();
                          ^~~~~~~~
../../src/authorizer/local/authorizer.cpp:482:29: error: jump to case label [-fpermissive]
         case authorization::VIEW_CONTAINER:
                             ^~~~~~~~~~~~~~
../../src/authorizer/local/authorizer.cpp:422:26: note:   crosses initialization of ‘Option<std::__cxx11::basic_string<char>
> taskUser’
           Option<string> taskUser = None();
                          ^~~~~~~~
../../src/authorizer/local/authorizer.cpp:496:29: error: jump to case label [-fpermissive]
         case authorization::SET_LOG_LEVEL:
                             ^~~~~~~~~~~~~
../../src/authorizer/local/authorizer.cpp:422:26: note:   crosses initialization of ‘Option<std::__cxx11::basic_string<char>
> taskUser’
           Option<string> taskUser = None();
                          ^~~~~~~~
../../src/authorizer/local/authorizer.cpp:500:29: error: jump to case label [-fpermissive]
         case authorization::UNKNOWN:
                             ^~~~~~~
../../src/authorizer/local/authorizer.cpp:422:26: note:   crosses initialization of ‘Option<std::__cxx11::basic_string<char>
> taskUser’
           Option<string> taskUser = None();
                          ^~~~~~~~
../../src/authorizer/local/authorizer.cpp:223:14: error: enumeration value ‘UNKNOWN’ not
handled in switch [-Werror=switch]
       switch (action_) {
              ^
../../src/authorizer/local/authorizer.cpp:223:14: error: enumeration value ‘VIEW_EXECUTOR’
not handled in switch [-Werror=switch]
../../src/authorizer/local/authorizer.cpp:223:14: error: enumeration value ‘LAUNCH_NESTED_CONTAINER’
not handled in switch [-Werror=switch]
../../src/authorizer/local/authorizer.cpp:223:14: error: enumeration value ‘LAUNCH_NESTED_CONTAINER_SESSION’
not handled in switch [-Werror=switch]
../../src/authorizer/local/authorizer.cpp:223:14: error: enumeration value ‘VIEW_CONTAINER’
not handled in switch [-Werror=switch]
../../src/authorizer/local/authorizer.cpp:223:14: error: enumeration value ‘SET_LOG_LEVEL’
not handled in switch [-Werror=switch]
cc1plus: all warnings being treated as errors
```


- Gastón


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57560/#review168822
-----------------------------------------------------------


On March 13, 2017, 6:19 p.m., Gastón Kleiman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57560/
> -----------------------------------------------------------
> 
> (Updated March 13, 2017, 6:19 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Removed unnecessary curly braces wrapping case statements.
> 
> 
> Diffs
> -----
> 
>   src/authorizer/local/authorizer.cpp 2227b241eab1606815fa6464e3d6b1345624fd22 
> 
> 
> Diff: https://reviews.apache.org/r/57560/diff/1/
> 
> 
> Testing
> -------
> 
> `make check` in Linux
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message