> 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 > >
