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


Looks like a reasonable start to a useful class. I feel like there should be 
some existing code/data-structure that would handle this use case, but it's not 
boost's PropertyTree, so I don't know what it would be. Several nits, but the 
biggest change would be switching over to use stout Path (although you might 
have to build it up to get value out of it).


3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 14)
<https://reviews.apache.org/r/37996/#comment157799>

    Why is this properties.hpp instead of pathinheritedproperties.hpp? Are 
there other kinds of properties you see going in here?



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 48)
<https://reviews.apache.org/r/37996/#comment157792>

    Comment should mention the thread-safety (or lack thereof) of this data 
structure.



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (lines 63 - 64)
<https://reviews.apache.org/r/37996/#comment157780>

    "non well formed" => "malformed"



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 67)
<https://reviews.apache.org/r/37996/#comment157779>

    Why use a string instead of a stout::Path?



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 118)
<https://reviews.apache.org/r/37996/#comment157782>

    s/not/no/



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 163)
<https://reviews.apache.org/r/37996/#comment157784>

    s/is binary operation/is a binary operation/



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (lines 189 - 
190)
<https://reviews.apache.org/r/37996/#comment157785>

    Only need 1 blank line here



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 191)
<https://reviews.apache.org/r/37996/#comment157786>

    s/exists/exist/



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 192)
<https://reviews.apache.org/r/37996/#comment157795>

    But it doesn't actually set the value, it just creates a node and lets the 
caller set the value with copy/move semantics.



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 256)
<https://reviews.apache.org/r/37996/#comment157793>

    s/three/tree/



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 276)
<https://reviews.apache.org/r/37996/#comment157796>

    I don't understand "make current an property holding node". What is 
"current"?
    Also, s/an property/a property/



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 329)
<https://reviews.apache.org/r/37996/#comment157797>

    "The root node is the only one without a parent."



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 377)
<https://reviews.apache.org/r/37996/#comment157798>

    Superfluous comment, since `init` is passed in.



3rdparty/libprocess/3rdparty/stout/tests/properties_tests.cpp (line 31)
<https://reviews.apache.org/r/37996/#comment157787>

    Bug? s/left/right/?



3rdparty/libprocess/3rdparty/stout/tests/properties_tests.cpp (line 45)
<https://reviews.apache.org/r/37996/#comment157788>

    s/checkes/checks/
    s/queriend/querying/



3rdparty/libprocess/3rdparty/stout/tests/properties_tests.cpp (line 47)
<https://reviews.apache.org/r/37996/#comment157789>

    s/parent/ancestor/?



3rdparty/libprocess/3rdparty/stout/tests/properties_tests.cpp (lines 68 - 71)
<https://reviews.apache.org/r/37996/#comment157790>

    Duplicate?



3rdparty/libprocess/3rdparty/stout/tests/properties_tests.cpp (line 111)
<https://reviews.apache.org/r/37996/#comment157791>

    s/accu/accumulator/ for readability. It's not like you have to type it much.


- Adam B


On Sept. 23, 2015, 4:17 a.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37996/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2015, 4:17 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3231
>     https://issues.apache.org/jira/browse/MESOS-3231
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Introduces the `PathInheritedProperties` class which allows to create a tree 
> where nodes can be tag with some property. This property is then inherited by 
> children nodes.
> 
> Two behaviors are implemented, overriding, i.e. Adding a property to the 
> child node of another node with a property already will result in the 
> ancestor property being lost. The second behavior, accumulating, takes a 
> function and accumulates
> properties of all ancestors.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 
> 76e1674e08bbe65a4fdf86731823a61f231d6d12 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 9e9c3119ad18f4cbc70c70095c71dc4fd19553df 
>   3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
> baa648aacfd5204c33e102b58126862729fbb612 
>   3rdparty/libprocess/3rdparty/stout/tests/properties_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37996/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Reply via email to