> On March 4, 2016, 12:59 p.m., Bernd Mathiske wrote:
> > "{}" is short, but cryptic. It is unclear what kind of entity is being 
> > passed here. "EMPTY" was not any better. "hashmap<SlaveID, Resources>()" at 
> > least revealed the type which hinted a little at the presumable purpose. A 
> > better identifier than "EMPTY" would beat all of the above. Maybe 
> > "NO_USED_RESOURCES"?
> > 
> > Could the parameter of the methods in question be supplied with a default 
> > value? Then no corresponding argument at all would appear in these tests.
> 
> Alexander Rukletsov wrote:
>     I'm a bit reluctant to adding a default value, because these are public 
> functions from an `Allocator` interface. In previous versions of the patch I 
> introduced verbose constants like `NO_USED_RESOURCES` and `NO_ALLOCATIONS` 
> but we opted for more cryptic but shorter `{}`. Functions `addSlave()` and 
> `addFramework()` are crucial to allocator tests, hence motivating people to 
> look at their signatures is not a bad idea : ).

@Bernd: I am confused why we'd want to be specific about the type of these 
default-constructed function call arguments here. I can understand that 
avoiding `auto` on an assignments LHS helps the reader anticipate e.g., the 
interface satisfied by some variable's type, but for arguments we do nothing 
with the value.

Moreover, in this case we construct empty sets and at least to me `{}` is a 
very intuitive notation for that.


- Benjamin


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


On March 3, 2016, 2 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44334/
> -----------------------------------------------------------
> 
> (Updated March 3, 2016, 2 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/44334/diff/
> 
> 
> Testing
> -------
> 
> Tested as a chain in https://reviews.apache.org/r/44336/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Reply via email to