----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20885/#review41863 -----------------------------------------------------------
Ship it! Looks great Isabel. Just some minor style nits, should be quick to cleanup then we'll get this committed. Thank you! 3rdparty/libprocess/3rdparty/stout/include/stout/base64.hpp <https://reviews.apache.org/r/20885/#comment75514> Add an extra space: namespace base64 { 3rdparty/libprocess/3rdparty/stout/include/stout/base64.hpp <https://reviews.apache.org/r/20885/#comment75515> We tend to try and use single letters (ala Haskell) or full words. So how about s/str/s/ here and for the parameter in 'decode' too? 3rdparty/libprocess/3rdparty/stout/include/stout/base64.hpp <https://reviews.apache.org/r/20885/#comment75516> Related to the comment above, how about s/ret/result/? Likewise below you use 'len' which could be replaced with 'length'. Same in 'decode' too please! 3rdparty/libprocess/3rdparty/stout/include/stout/base64.hpp <https://reviews.apache.org/r/20885/#comment75518> We put * and & next to the type, so s/char */char*/. Please do a quick sweep of your code and make these changes everywhere, thank you! 3rdparty/libprocess/3rdparty/stout/include/stout/base64.hpp <https://reviews.apache.org/r/20885/#comment75520> (1) We put spaces between 'while', 'if', 'for', etc. and the paren, so s/for(/for (/ here please. (2) We always use braces for our loops (for, while) and conditionals (if) unless they really can fit on one line. In all the cases you have here I think it's better to just add {} everywhere. Can you make both of these changes here and everywhere else please? Thanks Isabel! 3rdparty/libprocess/3rdparty/stout/include/stout/base64.hpp <https://reviews.apache.org/r/20885/#comment75523> Is 'in' meant to stand for 'index'? If so, let's use the full name please. 3rdparty/libprocess/3rdparty/stout/include/stout/base64.hpp <https://reviews.apache.org/r/20885/#comment75525> s/process/stout/ on these two lines too please! ;-) 3rdparty/libprocess/3rdparty/stout/include/stout/base64.hpp <https://reviews.apache.org/r/20885/#comment75524> You can kill this newline. - Benjamin Hindman On April 30, 2014, 9 a.m., Isabel Jimenez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20885/ > ----------------------------------------------------------- > > (Updated April 30, 2014, 9 a.m.) > > > Review request for mesos, Adam B and Benjamin Hindman. > > > Bugs: MESOS-1131 > https://issues.apache.org/jira/browse/MESOS-1131 > > > Repository: mesos-git > > > Description > ------- > > 1st Phase following Ben's comments on https://reviews.apache.org/r/19575/ > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/Makefile.am 7faa562 > 3rdparty/libprocess/3rdparty/stout/include/stout/base64.hpp PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/tests/base64_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/20885/diff/ > > > Testing > ------- > > > Thanks, > > Isabel Jimenez > >
