mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 70390: Added a test to ensure correct headroom accounting.
Date Mon, 08 Apr 2019 18:39:37 GMT


> On April 4, 2019, 10:02 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 6199-6200 (patched)
> > <https://reviews.apache.org/r/70390/diff/1/?file=2137502#file2137502line6199>
> >
> >     I realize it's not just this test, but this brace syntax but it seems less readable
than:
> >     
> >     const string CHILD_ROLE = "a/b";
> >     
> >     Does that not compile?
> 
> Meng Zhu wrote:
>     Changed to `const string CHILD_ROLE = "a/b";`
>     
>     I think, in general, the initialization list is preferred over the assignment when
initializing objects. But yeah, here the assignment seems more readable. Updated.

> I think, in general, the initialization list is preferred over the assignment when initializing
objects.

Hm.. the `=` syntax isn't invoking assignment operator when it's also a declaration of the
variable. It's going to invoke the following constructor FWICT:

```
basic_string( const CharT* s,
              const Allocator& alloc = Allocator() );
```

For the brace initializer, I think it's useful in cases where it's needed, like std::vector,
std::map, etc initilazation with entries. However, do you know what this code does for std::string?
At first glance I thought it would invoke this:

```
basic_string( std::initializer_list<CharT> ilist, 
              const Allocator& alloc = Allocator() );
```

But this seems to suggest callers like this:

```
string abc = {'a', 'b', 'c'};
```

No? I'm a bit puzzled by what `string abc{"abc"};` actually does..

Also, I think for the use of initializer list construction, we might as well use `=` to make
it more readable?

```
std::map<int, string> m = {{1, "one"}, {2, "2"}}; // seems more readable
// vs
std::map<int, string> m{{1, "one"}, {2, "2"}}; // seems more readable
```


- Benjamin


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


On April 4, 2019, 8:31 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70390/
> -----------------------------------------------------------
> 
> (Updated April 4, 2019, 8:31 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This test verifies that the headroom accounting is correct in the
> presence of subrole allocation. In particular, we need to ensure
> that a subrole's allocation is accounted only once (not multiple
> times in itself as well as all of its ancestors) when calculating
> available the quota headroom.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp c7edee2f49bfac0796c9506265d3bb766416ee63

> 
> 
> Diff: https://reviews.apache.org/r/70390/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message