> On April 21, 2015, 3:27 a.m., Joris Van Remoortere wrote: > > Hey Marco, great first patch! > > Some high level comments: > > - Can you check your editor settings to make sure you don't modify lines > > you don't intend? > > - Just do a quick look over the 'diff' section before publishing a review > > so can you see if everything being reviewed is intended (i.e. eclipse > > template) > > - If we're moving around large blocks of code, let's split out any > > refactoring / semantic changes into a seperate review. This way it's easier > > to see just which lines you actually modified vs. moved. It will get the > > code commited faster. (i.e. logging refactor). > > > > Can you make these changes and then I'll take a deeper look at > > framework.cpp :-)
1. apart from removing tabs (which follows the "Boy Scout Principle") my settings are just as you mention: I don't think my changes touched anything other than intended changes? The only areas I touched that were outside the scope was lining up the continuation slashes (`\ `) on the .am file (which was, honestly, pretty awful to look at :) 2. I did and it seemed to me to be ok? (Eclipse 'ignores' ought to be part of the .gitignore IMO as other developers who may be using Eclips will need it? why not?) 3. agreed - although it seemed to me such a minor non-functional change that it did not warrant its own review: and the original code IMO should have not gone past review anyway (violating DRY principle: so, as this was technically "new" code of mine, I'd be on the 'blame' hook for it) I will revert the check on the non-contains for the task, that is a 'material' change and can go in its own review. Please advise on the .gitignore changes and why those should not be part of the .gitignore. > On April 21, 2015, 3:27 a.m., Joris Van Remoortere wrote: > > .gitignore-template, lines 22-33 > > <https://reviews.apache.org/r/33376/diff/1/?file=937063#file937063line22> > > > > I think this got added to your commit accidentaly? If you want to add > > this to the repository we should make a separate JIRA and review for this. it was not accidental - but, really: do you need a review to add ignore files? it seems a bit overkill to me. > On April 21, 2015, 3:27 a.m., Joris Van Remoortere wrote: > > include/mesos/type_utils.hpp, line 184 > > <https://reviews.apache.org/r/33376/diff/1/?file=937064#file937064line184> > > > > I think we need an extra whitespace here. Can you check the style guide > > / other files? I'm actually guessing you meant around the `==` in `operator == (...)`? (you commented a blank line) You're right - fixed > On April 21, 2015, 3:27 a.m., Joris Van Remoortere wrote: > > include/mesos/type_utils.hpp, line 187 > > <https://reviews.apache.org/r/33376/diff/1/?file=937064#file937064line187> > > > > This comment doesn't seem to convey anything about what this code does, > > rather why it's part of this review request. Let's make it something more > > meaningful. > > > > Also, comments end with a period. right, I felt equally uneasy, but not sure how to address this, maybe a TODO? in Java, you have a clear distinction: ``` /** * This is a javadoc comment, a description of what a class is, what a method does */ // This is an implementation detail, will never show up in API docs ``` In C/C++ (in theory) you could use the same `/** */` but no one outside Visual Studio does. How do I add an implementation detail, editor comment to our code? Here, I've just removed it, but also note in passing that NONE of the methods here have anything "meaningful" as a comment... > On April 21, 2015, 3:27 a.m., Joris Van Remoortere wrote: > > src/Makefile.am, lines 302-304 > > <https://reviews.apache.org/r/33376/diff/1/?file=937065#file937065line302> > > > > I don't think you meant to change the spacing of all these lines > > (highlighted and rest of file). Can you check your editor settings to see > > if you're modifying them accidentally? the right-hand side continuation \ was all over the place and looked honestly awful: I follow the "boyscout principle" and just lined up quite a few of them around the *one* change I made (adding `framework.cpp`) - hopefully this should upset no one? - Marco ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33376/#review80894 ----------------------------------------------------------- On April 20, 2015, 10:38 p.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33376/ > ----------------------------------------------------------- > > (Updated April 20, 2015, 10:38 p.m.) > > > Review request for mesos and Joris Van Remoortere. > > > Bugs: MESOS-2633 > https://issues.apache.org/jira/browse/MESOS-2633 > > > Repository: mesos > > > Description > ------- > > Created new file framework.cpp containing all the methods' implementations > for the `Framework` class (declared in master/master.hpp) > > Declared `operator ==` for Task in type_utils.hpp > (it was implemented before, but not declared in the header file); > > Refactored all the LOG(WARNING) to a single utility method. > > > Diffs > ----- > > .gitignore-template 1a8e2a8677a29f06ba6386d56537a0013d38821c > include/mesos/type_utils.hpp cdf5864389a72002b538c263d70bcade2bdffa45 > src/Makefile.am d15a37365bcdd5c3906160b46b389635b38b1673 > src/master/framework.cpp PRE-CREATION > src/master/master.hpp c10e7c08c191acef9d31d98018a47a2a952a4dfc > > Diff: https://reviews.apache.org/r/33376/diff/ > > > Testing > ------- > > All tests (make check) pass. > > > Thanks, > > Marco Massenzio > >
