Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
400463002:
Class for allocating a chunk of memory for RenderPass (Closed)
DescriptionClass for allocating a chunk of memory for RenderPass
For DrawQuads and SharedQuadState, RenderPass used allocate them one by
one whenever needed. This new class helps RenderPass manages allocation
and iteration of those two types. This container allocates a chunk of
memory at one time and hands out raw pointers. It also provides iterator
and reverse iterators for going through its contents.
Unittest for ListContainer makes sure the raw pointers it hands out are
valid and iterator has same behavior as vector iterators.
Follows 398533002, and 404563005.
BUG=344962
Committed: https://crrev.com/6ae8c50c839a8c0d8fb399578256c754ed3ce39a
Cr-Commit-Position: refs/heads/master@{#296100}
Committed: https://crrev.com/d1f5016cb601daa67da3ca36eff3c8af1f35fa60
Cr-Commit-Position: refs/heads/master@{#296176}
Patch Set 1 #Patch Set 2 : fix compile error #Patch Set 3 : add erase function #Patch Set 4 : instead of vector of vector use vector of custom struct #Patch Set 5 : rebase #Patch Set 6 : index instead of iterators and store variables to make it faster (mixed with rebase sorry) #
Total comments: 4
Patch Set 7 : add to gn #
Total comments: 4
Patch Set 8 : move from header to source file #
Total comments: 72
Patch Set 9 : scoped ptr vector and also fix bug with reserve #Patch Set 10 : rebase #Patch Set 11 : unittest for destructor call and move everything inside and everything #Patch Set 12 : rm reserve #Patch Set 13 : next_list_allocation_size_ no more #Patch Set 14 : fix crash when insert after clear bug and add new unittest for that #Patch Set 15 : fix the fix #
Total comments: 20
Patch Set 16 : rebase #Patch Set 17 : address review comments #
Total comments: 69
Patch Set 18 : rebase #
Total comments: 4
Patch Set 19 : simple review comments addressed #Patch Set 20 : add comment for decrease capacity in erase #Patch Set 21 : template AllocateAndConstruct function #
Total comments: 12
Patch Set 22 : address review comments #
Total comments: 1
Patch Set 23 : rebase #Patch Set 24 : use name One and Two #Patch Set 25 : fix try bot build error because const iterator comparison is a pain #Patch Set 26 : attemp to fix trybot failures #Patch Set 27 : test with trybot #Patch Set 28 : try instan nested class when compile with msvc #Patch Set 29 : Position is not a class but a struct #Patch Set 30 : try add more instan in unittest for win component build #Patch Set 31 : aha cc export #
Total comments: 2
Patch Set 32 : fix linux asan #Patch Set 33 : use scoped_ptr<char[]> #
Total comments: 2
Patch Set 34 : use reset instead of assignment #
Messages
Total messages: 37 (4 generated)
Used in CL 448303002
https://codereview.chromium.org/400463002/diff/160001/cc/cc.gyp File cc/cc.gyp (right): https://codereview.chromium.org/400463002/diff/160001/cc/cc.gyp#newcode321 cc/cc.gyp:321: 'quads/list_container.cc', update cc/BUILD.gn too https://codereview.chromium.org/400463002/diff/160001/cc/cc_tests.gyp File cc/cc_tests.gyp (right): https://codereview.chromium.org/400463002/diff/160001/cc/cc_tests.gyp#newcode74 cc/cc_tests.gyp:74: 'quads/list_container_unittest.cc', ditto
https://codereview.chromium.org/400463002/diff/160001/cc/cc.gyp File cc/cc.gyp (right): https://codereview.chromium.org/400463002/diff/160001/cc/cc.gyp#newcode321 cc/cc.gyp:321: 'quads/list_container.cc', On 2014/08/20 23:32:49, jamesr wrote: > update cc/BUILD.gn too Done. https://codereview.chromium.org/400463002/diff/160001/cc/cc_tests.gyp File cc/cc_tests.gyp (right): https://codereview.chromium.org/400463002/diff/160001/cc/cc_tests.gyp#newcode74 cc/cc_tests.gyp:74: 'quads/list_container_unittest.cc', On 2014/08/20 23:32:49, jamesr wrote: > ditto Done.
Thanks. build system stuff lgtm but I didn't review the C++
https://codereview.chromium.org/400463002/diff/180001/cc/quads/list_container.cc File cc/quads/list_container.cc (right): https://codereview.chromium.org/400463002/diff/180001/cc/quads/list_container... cc/quads/list_container.cc:32: last_list_(NULL) { this doesn't init list_count_? https://codereview.chromium.org/400463002/diff/180001/cc/quads/list_container.h File cc/quads/list_container.h (right): https://codereview.chromium.org/400463002/diff/180001/cc/quads/list_container... cc/quads/list_container.h:9: #include "base/macros.h" empty line above this https://codereview.chromium.org/400463002/diff/180001/cc/quads/list_container... cc/quads/list_container.h:20: class CC_EXPORT ListContainer { can you nest this inside the ListContainerAccessor (maybe name that one ListContainer and this one something else). You can forward declare the class in the .h and define it entirely in the .cc to hide it away. https://codereview.chromium.org/400463002/diff/180001/cc/quads/list_container... cc/quads/list_container.h:26: // Since this class never reallocate its memory, calling reserve will set reallocates
Change: - rename classes, e.g. ListContainerAccessor<T> -> ListContainer<T> - forward declare class allocates raw memory in char - moved Position (base for iterators) out of class allocating raw memory - move all implementation to source file - minor cleanup in comments, and unittest
https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container.cc File cc/quads/list_container.cc (right): https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:36: : current_list_size_(kDefaultNumElementTypesToReserve), next_list_allocation_size? https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:55: (*i)->~InnerList(); what actually deletes the InnerList? Can storage_ be a ScopedPtrVector? https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:70: void Reserve(size_t element_count) { current_list_size_ = element_count; } how can you unit test that reserve does the right thing? maybe add a capacity() method that says how much memory is currently allocated? something else? What if your last list allocation was 300, and you want to reserve space for 10 elements. Your next list shouldn't be only size 10 should it? I would expect it to still be 600. My expectation is that reserve does an allocation immediately, if needed, to ensure there are >= N spaces free in the structure. I don't see you using this method at all in your next patch. Maybe just don't add this now, and later if we need it add it? https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:87: void Erase(char* position) { DCHECK that position < end https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:92: char* start = position; start = position + step; (don't need to split this across 2 lines) https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:96: std::copy(start, end, result); (start, end, position)? https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:102: bool Full() { return capacity == size; } IsFull https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:104: void* AddElement() { DCHECK_LT(size, capacity) at the start of this https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:122: (*i)->~InnerList(); what deletes the InnerList? same Q from the destructor https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:153: list->data = new char[current_list_size_ * element_size_]; You could use the storage_.back()->capacity() here. Maybe you don't need this current_list_size_ member at all? Especially since it seems Reserve isn't needed for now? Do you need the reserving constructor now either? If you do need to keep reserve, then I would suggest changing things up so that Reserve() immediately allocates, like CharAllocator(size_t n) { Reserve(n); } Reserve(size_t n) { size_t space = storage_.back()->capacity() - storage_.back()->size(); if (space >= n) return; AllocateNewList(space - n); } AllocateNewList(size_t at_least) { size_t next_capacity = kDefaultFirstCapacity; if (!storage_.empty()) next_capacity = storage_.back()->capacity() * 2; next_capacity = std::max(next_capacity, at_least); ..... } Allocate() { if (need new list) AllocateNewList(0); ... } This way you avoid the current_list_size_ member variable, and your reserve doesn't clobber the next allocation size. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:156: list->step = element_size_; do you need the step variable here? it's always the same as element_size_. can you just pass that around as needed from the CharAllocator? https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:160: InnerList* LastList() { return last_list_; } { InnerList* back = storage_.back(); return back->data[back->size - 1]; } No need for the last_list_ member is there? https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:208: return vector_index == other.vector_index && DCHECK that ptr_to_containers match https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:215: return !operator==(other); nit: return !(*this == other); https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:266: if (!data_->empty()) { just call clear() here? https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:268: i->~BaseElementType(); can you add unit tests showing this is called when you clear(), and when you destroy ListContainer? to do this you can put these type things in the list, and point them to some bools owned by the test itself: class Element { public: Element() : destructor_called_(NULL) {} ~Element() { *destructor_called_ = true; } void set_destructor_called(bool* d) { destructor_called_ = d; } private: bool* destructor_called_; }; https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:271: delete data_; can data_ be in a scoped_ptr so we don't have to delete it? https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:275: void ListContainer<BaseElementType>::reserve(size_t size) { put these methods in the order they appear in the header, ie below Allocate() https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:303: item->~BaseElementType(); can you unit test that this destructor happens too? https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:355: size_t last_id = data_->get_list_count() - 1; The end() is supposed to be 1 past the end of the structure. This looks like looping while it != end() would skip the last element? Ditto for the rend()s? Can you add checks to your iterator unit tests at the end that every element was visited? https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:488: return static_cast<const BaseElementType*>( casting to void* then to Foo* seems like a dance to reinterpret_cast without calling it what it is. why not just reinterpret_cast? https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container.h File cc/quads/list_container.h (right): https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:17: class ListContainerCharAllocator; Did you say you were going to nest this inside ListContainer yesterday? https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:22: struct CC_EXPORT PositionInListContainerCharAllocator { And this? https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:52: // This class is used by RenderPass. It is a container type that handles nit: class level comment on top of the class rather than inside it https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:53: // allocating memory for new element and traverse through elements with either elements and traversing https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:54: // iterator or reverse iterator. Since this container hands out raw pointer of raw pointers https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:56: // memory so those raw pointer will continue to be valid. pointers https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:57: // This class is used to contain SharedQuadState or DrawQuad. Since size of Since the size of each DrawQuad https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:58: // DrawQuad varies, this class allocates kLargestDrawQuad. ...varies, to hold DrawQuads, the BaseElementType should instead be kLargestDrawQuad. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:61: ListContainer(); can you add a comment explaining when you would use each of these 2 constructors and explain what size the parameterless constructor uses vs the size_t one? https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:70: Iterator(const Iterator& other); if you define a copy constructor, you need to define operator= too. optionally just omit both and you get the default ones. same comment for each iterator and the Position class. It looks to me like the defaults should work for you? https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:77: Iterator operator++(int); why do you have an operator++ that takes an argument for each of these? https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:130: // As can be seen from the name, after called, all raw pointer that has been pointers that have been https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:131: // handed out is no longer valid. Use with caution. are no longer https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:132: void eraseAndInvalidateAllPointers(Iterator position); EraseAnd.. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:147: BaseElementType* Allocate(); can you comment saying where in the structure/list the new element is allocated (ie back) https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... File cc/quads/list_container_unittest.cc (right): https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container_unittest.cc:68: list.reserve(2); I think with the changes suggested to reserving (or getting rid of it), you probably need to add a capacity() and reserve and/or allocate members until the capacity changes to convince the reader of the test that you have multiple vectors going on.
PTAL. I added more guards for reserve() but hasn't removed it. My assumption for reserve() is: - total number of elements needed is known when calling it. - used when constructor is already called but want to adjust how much space to reserve. - probably only called once before allocation starts? - hopefully after reserve(size) is called, allocate new chunk of memory only once. As mentioned in one of the comments, reserve() is most useful in cc_messages with both SQS and DQ, where both list sizes are known and RP constructor (that calls SQSList and DQList constructor) only takes in one parameter (num_of_layers, used to guess size of SQSList). https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container.cc File cc/quads/list_container.cc (right): https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:36: : current_list_size_(kDefaultNumElementTypesToReserve), On 2014/08/26 15:55:29, danakj wrote: > next_list_allocation_size? Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:55: (*i)->~InnerList(); On 2014/08/26 15:55:30, danakj wrote: > what actually deletes the InnerList? Can storage_ be a ScopedPtrVector? Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:70: void Reserve(size_t element_count) { current_list_size_ = element_count; } On 2014/08/26 15:55:30, danakj wrote: > how can you unit test that reserve does the right thing? maybe add a capacity() > method that says how much memory is currently allocated? something else? > > What if your last list allocation was 300, and you want to reserve space for 10 > elements. Your next list shouldn't be only size 10 should it? I would expect it > to still be 600. My expectation is that reserve does an allocation immediately, > if needed, to ensure there are >= N spaces free in the structure. > > I don't see you using this method at all in your next patch. Maybe just don't > add this now, and later if we need it add it? I've updated code with reserve so it will only change next list's allocation when there is not enough free space left. Reserve would be good for using inside cc_message, when you know the SQS and DQ list size and you want to reserve size after constructor, and calling reserve would make sure ListContainer only allo once. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:87: void Erase(char* position) { On 2014/08/26 15:55:29, danakj wrote: > DCHECK that position < end Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:92: char* start = position; On 2014/08/26 15:55:30, danakj wrote: > start = position + step; (don't need to split this across 2 lines) Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:96: std::copy(start, end, result); On 2014/08/26 15:55:29, danakj wrote: > (start, end, position)? Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:102: bool Full() { return capacity == size; } On 2014/08/26 15:55:30, danakj wrote: > IsFull Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:104: void* AddElement() { On 2014/08/26 15:55:30, danakj wrote: > DCHECK_LT(size, capacity) at the start of this Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:122: (*i)->~InnerList(); On 2014/08/26 15:55:30, danakj wrote: > what deletes the InnerList? same Q from the destructor Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:156: list->step = element_size_; On 2014/08/26 15:55:29, danakj wrote: > do you need the step variable here? it's always the same as element_size_. can > you just pass that around as needed from the CharAllocator? Before the main concern was to allow Position access to InnerList because using iterator does not go through CharAllo, now using index Position would be able to get to CharAllo so is it achievable now. I'd still say pass in an element_size_ and just use that inside seems easier than making sure all calls to InnerList goes through the correct CharAllo. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:160: InnerList* LastList() { return last_list_; } On 2014/08/26 15:55:30, danakj wrote: > { > InnerList* back = storage_.back(); > return back->data[back->size - 1]; > } > > No need for the last_list_ member is there? Was using this to keep track and avoid calculation each time before I started using index instead iterator. Attempted to keep track of pointer instead of calculated every time. Will remove. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:208: return vector_index == other.vector_index && On 2014/08/26 15:55:29, danakj wrote: > DCHECK that ptr_to_containers match Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:215: return !operator==(other); On 2014/08/26 15:55:29, danakj wrote: > nit: return !(*this == other); Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:266: if (!data_->empty()) { On 2014/08/26 15:55:30, danakj wrote: > just call clear() here? This guards for when data_ is empty and begin() would be NULL. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:268: i->~BaseElementType(); On 2014/08/26 15:55:29, danakj wrote: > can you add unit tests showing this is called when you clear(), and when you > destroy ListContainer? > > to do this you can put these type things in the list, and point them to some > bools owned by the test itself: > > class Element { > public: > Element() : destructor_called_(NULL) {} > ~Element() { *destructor_called_ = true; } > void set_destructor_called(bool* d) { destructor_called_ = d; } > private: > bool* destructor_called_; > }; Done use gmock EXPECT_CALL. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:271: delete data_; On 2014/08/26 15:55:29, danakj wrote: > can data_ be in a scoped_ptr so we don't have to delete it? Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:275: void ListContainer<BaseElementType>::reserve(size_t size) { On 2014/08/26 15:55:30, danakj wrote: > put these methods in the order they appear in the header, ie below Allocate() Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:303: item->~BaseElementType(); On 2014/08/26 15:55:29, danakj wrote: > can you unit test that this destructor happens too? Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:355: size_t last_id = data_->get_list_count() - 1; On 2014/08/26 15:55:29, danakj wrote: > The end() is supposed to be 1 past the end of the structure. This looks like > looping while it != end() would skip the last element? Ditto for the rend()s? > > Can you add checks to your iterator unit tests at the end that every element was > visited? The last element would be (data_, last_id, [actual pointer]), while end is (data_, last_id, NULL). Added in unittest that iterating through ListContainer is the same as iterating through a normal vector. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.cc:488: return static_cast<const BaseElementType*>( On 2014/08/26 15:55:29, danakj wrote: > casting to void* then to Foo* seems like a dance to reinterpret_cast without > calling it what it is. why not just reinterpret_cast? Was under (wrong) impression reinterpret_cast is slow and should be avoid. Will change. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container.h File cc/quads/list_container.h (right): https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:17: class ListContainerCharAllocator; On 2014/08/26 15:55:31, danakj wrote: > Did you say you were going to nest this inside ListContainer yesterday? Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:22: struct CC_EXPORT PositionInListContainerCharAllocator { On 2014/08/26 15:55:31, danakj wrote: > And this? Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:52: // This class is used by RenderPass. It is a container type that handles On 2014/08/26 15:55:31, danakj wrote: > nit: class level comment on top of the class rather than inside it Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:53: // allocating memory for new element and traverse through elements with either On 2014/08/26 15:55:31, danakj wrote: > elements and traversing Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:54: // iterator or reverse iterator. Since this container hands out raw pointer of On 2014/08/26 15:55:31, danakj wrote: > raw pointers Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:56: // memory so those raw pointer will continue to be valid. On 2014/08/26 15:55:30, danakj wrote: > pointers Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:57: // This class is used to contain SharedQuadState or DrawQuad. Since size of On 2014/08/26 15:55:30, danakj wrote: > Since the size of each DrawQuad Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:58: // DrawQuad varies, this class allocates kLargestDrawQuad. On 2014/08/26 15:55:30, danakj wrote: > ...varies, to hold DrawQuads, the BaseElementType should instead be > kLargestDrawQuad. Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:61: ListContainer(); On 2014/08/26 15:55:30, danakj wrote: > can you add a comment explaining when you would use each of these 2 constructors > and explain what size the parameterless constructor uses vs the size_t one? Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:70: Iterator(const Iterator& other); On 2014/08/26 15:55:31, danakj wrote: > if you define a copy constructor, you need to define operator= too. optionally > just omit both and you get the default ones. same comment for each iterator and > the Position class. It looks to me like the defaults should work for you? Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:77: Iterator operator++(int); On 2014/08/26 15:55:30, danakj wrote: > why do you have an operator++ that takes an argument for each of these? operator++(int) is post increment, and operator++() is pre increment. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:130: // As can be seen from the name, after called, all raw pointer that has been On 2014/08/26 15:55:30, danakj wrote: > pointers that have been Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:131: // handed out is no longer valid. Use with caution. On 2014/08/26 15:55:31, danakj wrote: > are no longer Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:132: void eraseAndInvalidateAllPointers(Iterator position); On 2014/08/26 15:55:30, danakj wrote: > EraseAnd.. Done. https://codereview.chromium.org/400463002/diff/200001/cc/quads/list_container... cc/quads/list_container.h:147: BaseElementType* Allocate(); On 2014/08/26 15:55:31, danakj wrote: > can you comment saying where in the structure/list the new element is allocated > (ie back) Done.
On Wed, Sep 3, 2014 at 7:58 PM, <weiliangc@chromium.org> wrote: > Reviewers: danakj, jamesr, > > Message: > PTAL. > > I added more guards for reserve() but hasn't removed it. > > My assumption for reserve() is: > - total number of elements needed is known when calling it. > - used when constructor is already called but want to adjust how much > space to > reserve. > - probably only called once before allocation starts? > - hopefully after reserve(size) is called, allocate new chunk of memory > only > once. > > As mentioned in one of the comments, reserve() is most useful in > cc_messages > with both SQS and DQ, where both list sizes are known and RP constructor > (that > calls SQSList and DQList constructor) only takes in one parameter > (num_of_layers, used to guess size of SQSList). > As per offline, I think reserve() isn't a good name for it since that differs from what reserve does in other classes, and if it's only called right after constructor, we should find a way to just roll it into the constructor. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Removed reserve and next_list_allocation_size. :)
https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container.h File cc/quads/list_container.h (right): https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:16: // This class is used by RenderPass. It is a container type that handles other classes could use it, maybe we shouldn't define the class by what class will use it (right now). https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:17: // allocating memory for new elements and traversing through elements with allocating contiguous memory for elements? https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:29: // constructor takes in the largest memory size required for all the derived s/Input of this constructor takes in the/|max_size_for_derived_class| is the/ https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:36: // This constructor control default allocation chunk size, and is used when as a user of this class, i'm not sure what "control default allocation chunk size" means or what it's important. How about instead say this reserves the requested memory up front so that only a single allocation is needed https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:40: size_t default_allocation_chunk_size); is this a size in bytes or a number of elements (each of size max_size_for_derived_class). maybe this name could describe that instead as "reserve_foo_bar_units"? https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:84: Iterator(ListContainerCharAllocator* container, I'm not sure if this needs to be const? Or if the parent class doesn't need to be templated. It seems like the latter. Also please unit test const iterators https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:90: Iterator operator++(int); can you add a variable name to meet the style guide? Maybe |unused_post_increment|? https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:140: // As can be seen from the name, after called, all raw pointers that have been s/As can be seen from the name, after called, /When called, / https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:164: size_t AvailableSizeWithoutAnotherAllocation() const; is this just for tests? If so, add a ForTesting suffix https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:168: }; DISALLOW_COPY_AND_ASSIGN
Address review comments :D https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container.h File cc/quads/list_container.h (right): https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:16: // This class is used by RenderPass. It is a container type that handles On 2014/09/09 17:29:13, danakj wrote: > other classes could use it, maybe we shouldn't define the class by what class > will use it (right now). Done. https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:17: // allocating memory for new elements and traversing through elements with On 2014/09/09 17:29:13, danakj wrote: > allocating contiguous memory for elements? Done. https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:29: // constructor takes in the largest memory size required for all the derived On 2014/09/09 17:29:13, danakj wrote: > s/Input of this constructor takes in the/|max_size_for_derived_class| is the/ Done. https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:36: // This constructor control default allocation chunk size, and is used when On 2014/09/09 17:29:13, danakj wrote: > as a user of this class, i'm not sure what "control default allocation chunk > size" means or what it's important. How about instead say this reserves the > requested memory up front so that only a single allocation is needed Done. https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:40: size_t default_allocation_chunk_size); On 2014/09/09 17:29:13, danakj wrote: > is this a size in bytes or a number of elements (each of size > max_size_for_derived_class). maybe this name could describe that instead as > "reserve_foo_bar_units"? Done. https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:84: Iterator(ListContainerCharAllocator* container, On 2014/09/09 17:29:13, danakj wrote: > I'm not sure if this needs to be const? Or if the parent class doesn't need to > be templated. It seems like the latter. > > Also please unit test const iterators Done. https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:90: Iterator operator++(int); On 2014/09/09 17:29:13, danakj wrote: > can you add a variable name to meet the style guide? Maybe > |unused_post_increment|? Done. https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:140: // As can be seen from the name, after called, all raw pointers that have been On 2014/09/09 17:29:13, danakj wrote: > s/As can be seen from the name, after called, /When called, / Done. https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:164: size_t AvailableSizeWithoutAnotherAllocation() const; On 2014/09/09 17:29:13, danakj wrote: > is this just for tests? If so, add a ForTesting suffix Done. https://codereview.chromium.org/400463002/diff/340001/cc/quads/list_container... cc/quads/list_container.h:168: }; On 2014/09/09 17:29:13, danakj wrote: > DISALLOW_COPY_AND_ASSIGN Done.
https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container.cc File cc/quads/list_container.cc (right): https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:20: // List Container Char Allocator nit: no spaces https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:54: // ListContainerCharAllocator::InnerList can you move this class to the very top of the outer class, so each class has all its contents together? https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:57: char* data; scoped_ptr<char*>? https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:58: size_t capacity; leave a comment saying what this is and the units (ie it's not bytes) https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:59: size_t size; leave a comment saying what this is and the units (ie it's not bytes) https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:60: size_t step; leave a comment saying what this is https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:63: data = NULL; use constructor initializers https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:73: DCHECK_LE(position, back()); wanna also check GE(position, data)? https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:76: std::copy(start, end, position); use end() instead of computing again here? https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:79: --capacity; you actually have an empty element left over here, do you mean to reduce the capacity? If so, can you add a comment to the public erase method that it does not actually delete any memory? https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:87: return data + (size - 1) * step; return back()? https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:90: size_t available() const { return capacity - size; } NumElementsAvailable Maybe move this up beside IsFull? They're kinda siblings. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:91: for a bunch of oneline methods you could skip whitespace between them.. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:92: char* end() const { return data + size * step; } End() this is private API no reason to break the style guide and look like an STL class here. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:94: char* back() const { return data + (size - 1) * step; } Back() or LastElement() https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:96: ~InnerList() { delete[] data; } destructor not needed if you use scoped_ptr<char*> https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:97: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:99: size_t size() const { return size_; } ..but for larger methods please put whitespace between https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:100: size_t capacity() const { Capacity https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:110: bool empty() const { return size() == 0; } move this up right below size()? Also IsEmpty https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:112: void clear() { Clear https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:123: storage_[position.vector_index]->Erase(position.item_iterator); leave a TODO to free the inner list if it's empty? https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:127: size_t get_list_count() const { return list_count_; } list_count() or GetListCount() and move this up by size() with other getters? https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:130: DCHECK(id < list_count_); DCHECK_LT https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:146: size_t AvailableSizeInLastList() const { return last_list_->available(); } NumAvailableElementsInLastList()? https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:148: ScopedPtrVector<InnerList> storage_; these are public? https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:202: item_iterator += list->step; this isn't so great, it could technically wrap around and give you a wrong answer on the next line. it would be much better to do the comparison first then increment. maybe a temp bool to use on the next line below? https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:203: if (!(item_iterator < list->end())) { Use == https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:220: item_iterator -= list->step; same here, compare first https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:236: : data_(make_scoped_ptr( is make_scoped_ptr needed for a constructor? https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:257: if (!data_->empty()) { no need to if(), the for loop will just do nothing, right? https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:386: if (!data_->empty()) { no need to if(), the for loop will just do nothing, right? https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container.h File cc/quads/list_container.h (right): https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.h:80: Iterator operator++(int /*unused_post_increment*/); nit: remove the /* */ just leave the name there (chromium style is to always have a variable name) https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.h:146: BaseElementType* Allocate(); template <typename Type> BaseElementType* Allocate() and then this thing can actually call the constructor. you could DCHECK that sizeof(Type) <= sizeof(data_->element_size_) too. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... File cc/quads/list_container_unittest.cc (right): https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container_unittest.cc:48: ListContainer<DrawQuad> list(sizeof(kLargestDrawQuad)); with the templated Allocate() we should be able to avoid in place allocators altogether?
Address review comments with a large list of Done. :) https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container.cc File cc/quads/list_container.cc (right): https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:20: // List Container Char Allocator On 2014/09/15 21:45:30, danakj wrote: > nit: no spaces Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:54: // ListContainerCharAllocator::InnerList On 2014/09/15 21:45:31, danakj wrote: > can you move this class to the very top of the outer class, so each class has > all its contents together? Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:57: char* data; On 2014/09/15 21:45:31, danakj wrote: > scoped_ptr<char*>? scoped_ptr<char> Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:58: size_t capacity; On 2014/09/15 21:45:31, danakj wrote: > leave a comment saying what this is and the units (ie it's not bytes) Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:59: size_t size; On 2014/09/15 21:45:30, danakj wrote: > leave a comment saying what this is and the units (ie it's not bytes) Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:60: size_t step; On 2014/09/15 21:45:30, danakj wrote: > leave a comment saying what this is Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:63: data = NULL; On 2014/09/15 21:45:31, danakj wrote: > use constructor initializers Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:73: DCHECK_LE(position, back()); On 2014/09/15 21:45:30, danakj wrote: > wanna also check GE(position, data)? Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:76: std::copy(start, end, position); On 2014/09/15 21:45:31, danakj wrote: > use end() instead of computing again here? Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:87: return data + (size - 1) * step; On 2014/09/15 21:45:30, danakj wrote: > return back()? Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:90: size_t available() const { return capacity - size; } On 2014/09/15 21:45:30, danakj wrote: > NumElementsAvailable > > Maybe move this up beside IsFull? They're kinda siblings. Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:91: On 2014/09/15 21:45:31, danakj wrote: > for a bunch of oneline methods you could skip whitespace between them.. Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:92: char* end() const { return data + size * step; } On 2014/09/15 21:45:30, danakj wrote: > End() > > this is private API no reason to break the style guide and look like an STL > class here. Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:94: char* back() const { return data + (size - 1) * step; } On 2014/09/15 21:45:31, danakj wrote: > Back() or LastElement() Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:96: ~InnerList() { delete[] data; } On 2014/09/15 21:45:30, danakj wrote: > destructor not needed if you use scoped_ptr<char*> Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:97: }; On 2014/09/15 21:45:30, danakj wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:99: size_t size() const { return size_; } On 2014/09/15 21:45:32, danakj wrote: > ..but for larger methods please put whitespace between Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:100: size_t capacity() const { On 2014/09/15 21:45:31, danakj wrote: > Capacity Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:110: bool empty() const { return size() == 0; } On 2014/09/15 21:45:30, danakj wrote: > move this up right below size()? Also IsEmpty Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:112: void clear() { On 2014/09/15 21:45:30, danakj wrote: > Clear Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:123: storage_[position.vector_index]->Erase(position.item_iterator); On 2014/09/15 21:45:31, danakj wrote: > leave a TODO to free the inner list if it's empty? Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:127: size_t get_list_count() const { return list_count_; } On 2014/09/15 21:45:31, danakj wrote: > list_count() or GetListCount() > > and move this up by size() with other getters? Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:146: size_t AvailableSizeInLastList() const { return last_list_->available(); } On 2014/09/15 21:45:31, danakj wrote: > NumAvailableElementsInLastList()? Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:148: ScopedPtrVector<InnerList> storage_; On 2014/09/15 21:45:32, danakj wrote: > these are public? Changed to private. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:202: item_iterator += list->step; On 2014/09/15 21:45:31, danakj wrote: > this isn't so great, it could technically wrap around and give you a wrong > answer on the next line. > > it would be much better to do the comparison first then increment. maybe a temp > bool to use on the next line below? Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:203: if (!(item_iterator < list->end())) { On 2014/09/15 21:45:32, danakj wrote: > Use == Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:220: item_iterator -= list->step; On 2014/09/15 21:45:30, danakj wrote: > same here, compare first Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:236: : data_(make_scoped_ptr( On 2014/09/15 21:45:30, danakj wrote: > is make_scoped_ptr needed for a constructor? Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:257: if (!data_->empty()) { On 2014/09/15 21:45:32, danakj wrote: > no need to if(), the for loop will just do nothing, right? Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:386: if (!data_->empty()) { On 2014/09/15 21:45:32, danakj wrote: > no need to if(), the for loop will just do nothing, right? Done. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container.h File cc/quads/list_container.h (right): https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.h:80: Iterator operator++(int /*unused_post_increment*/); On 2014/09/15 21:45:32, danakj wrote: > nit: remove the /* */ just leave the name there > > (chromium style is to always have a variable name) Done.
https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container.cc File cc/quads/list_container.cc (right): https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.cc:79: --capacity; On 2014/09/15 21:45:31, danakj wrote: > you actually have an empty element left over here, do you mean to reduce the > capacity? If so, can you add a comment to the public erase method that it does > not actually delete any memory? This is mostly I feel awkward if an element if erased from InnerList that is not last, and capacity does not decrease, that InnerList would be not full and has no way to be full since Allocate only looks at last InnerList. Added comments.
https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... File cc/quads/list_container_unittest.cc (right): https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container_unittest.cc:48: ListContainer<DrawQuad> list(sizeof(kLargestDrawQuad)); On 2014/09/15 21:45:32, danakj wrote: > with the templated Allocate() we should be able to avoid in place allocators > altogether? Problem with tempalte Allocate(): The place to instatiate Allocate<MockDrawQuad>() and Allocate<SimpleDrawQuad>() would have to be: - in unittest file, since only unittest file has the two classes defined. - in ListContainer.cc file since explicit instantiate need to be in the source file.
Addressed all reviews comments. https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container.h File cc/quads/list_container.h (right): https://codereview.chromium.org/400463002/diff/380001/cc/quads/list_container... cc/quads/list_container.h:146: BaseElementType* Allocate(); On 2014/09/15 21:45:32, danakj wrote: > template <typename Type> BaseElementType* Allocate() > > and then this thing can actually call the constructor. > > you could DCHECK that sizeof(Type) <= sizeof(data_->element_size_) too. Done.
LGTM with some test comments https://codereview.chromium.org/400463002/diff/400001/cc/quads/list_container... File cc/quads/list_container_unittest.cc (right): https://codereview.chromium.org/400463002/diff/400001/cc/quads/list_container... cc/quads/list_container_unittest.cc:299: } can you EXPECT_EQ(num_iters_in_vector, num_iters_in_list)? https://codereview.chromium.org/400463002/diff/400001/cc/quads/list_container... cc/quads/list_container_unittest.cc:418: ++iter; what is this about? https://codereview.chromium.org/400463002/diff/460001/cc/quads/list_container.cc File cc/quads/list_container.cc (right): https://codereview.chromium.org/400463002/diff/460001/cc/quads/list_container... cc/quads/list_container.cc:33: // capacity represents how many elements in total the memory can hold. The // The number of elements in total the... https://codereview.chromium.org/400463002/diff/460001/cc/quads/list_container... cc/quads/list_container.cc:37: // size represents how many elements have been put into this list. // The number of elements that have.. https://codereview.chromium.org/400463002/diff/460001/cc/quads/list_container... cc/quads/list_container.cc:39: // step represents how big each element is in bytes. This is used to move // The size of each element in bytes... https://codereview.chromium.org/400463002/diff/460001/cc/quads/list_container... cc/quads/list_container.cc:105: size_t Size() const { return size_; } nit: this could be size() since it's a simple accessor https://codereview.chromium.org/400463002/diff/460001/cc/quads/list_container... File cc/quads/list_container_unittest.cc (right): https://codereview.chromium.org/400463002/diff/460001/cc/quads/list_container... cc/quads/list_container_unittest.cc:47: TEST(ListContainerTest, DestructorCalled) { can you add some ConstructorCalled ones also? https://codereview.chromium.org/400463002/diff/460001/cc/quads/list_container... cc/quads/list_container_unittest.cc:130: EXPECT_TRUE(list.empty()); check size = 0 here too?
address all review comments :) https://codereview.chromium.org/400463002/diff/400001/cc/quads/list_container... File cc/quads/list_container_unittest.cc (right): https://codereview.chromium.org/400463002/diff/400001/cc/quads/list_container... cc/quads/list_container_unittest.cc:299: } On 2014/09/19 19:43:37, danakj wrote: > can you EXPECT_EQ(num_iters_in_vector, num_iters_in_list)? Done. https://codereview.chromium.org/400463002/diff/400001/cc/quads/list_container... cc/quads/list_container_unittest.cc:418: ++iter; On 2014/09/19 19:43:37, danakj wrote: > what is this about? =-= Even reading git log this still doesn't make sense... Removed. https://codereview.chromium.org/400463002/diff/460001/cc/quads/list_container.cc File cc/quads/list_container.cc (right): https://codereview.chromium.org/400463002/diff/460001/cc/quads/list_container... cc/quads/list_container.cc:33: // capacity represents how many elements in total the memory can hold. The On 2014/09/19 19:43:37, danakj wrote: > // The number of elements in total the... Done. https://codereview.chromium.org/400463002/diff/460001/cc/quads/list_container... cc/quads/list_container.cc:37: // size represents how many elements have been put into this list. On 2014/09/19 19:43:37, danakj wrote: > // The number of elements that have.. Done. https://codereview.chromium.org/400463002/diff/460001/cc/quads/list_container... cc/quads/list_container.cc:39: // step represents how big each element is in bytes. This is used to move On 2014/09/19 19:43:37, danakj wrote: > // The size of each element in bytes... Done. https://codereview.chromium.org/400463002/diff/460001/cc/quads/list_container... cc/quads/list_container.cc:105: size_t Size() const { return size_; } On 2014/09/19 19:43:37, danakj wrote: > nit: this could be size() since it's a simple accessor Done. https://codereview.chromium.org/400463002/diff/460001/cc/quads/list_container... File cc/quads/list_container_unittest.cc (right): https://codereview.chromium.org/400463002/diff/460001/cc/quads/list_container... cc/quads/list_container_unittest.cc:47: TEST(ListContainerTest, DestructorCalled) { On 2014/09/19 19:43:37, danakj wrote: > can you add some ConstructorCalled ones also? Done. https://codereview.chromium.org/400463002/diff/460001/cc/quads/list_container... cc/quads/list_container_unittest.cc:130: EXPECT_TRUE(list.empty()); On 2014/09/19 19:43:37, danakj wrote: > check size = 0 here too? Done.
LGTM https://codereview.chromium.org/400463002/diff/480001/cc/quads/list_container... File cc/quads/list_container_unittest.cc (right): https://codereview.chromium.org/400463002/diff/480001/cc/quads/list_container... cc/quads/list_container_unittest.cc:44: class SimpleDrawQuadConstructMagicNumberFortyTwo : public SimpleDrawQuad { This is super hard to read such a loooong name :) How about just SimpleDrawQuadConstructMagicOne and ...Two ? The constants inside the constructors will speak for themselves.
The CQ bit was checked by weiliangc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/400463002/660001
Message was sent while issue was closed.
Committed patchset #31 (id:660001) as 3c1269bc10a759d9f28aa3079499853a8b19cefe
Message was sent while issue was closed.
Patchset 31 (id:??) landed as https://crrev.com/6ae8c50c839a8c0d8fb399578256c754ed3ce39a Cr-Commit-Position: refs/heads/master@{#296100}
Message was sent while issue was closed.
A revert of this CL (patchset #31 id:660001) has been created in https://codereview.chromium.org/595733002/ by weiliangc@chromium.org. The reason for reverting is: Broke cc_unittest for Linux ASan bot.
Linux ASan bot fail is because of mismatch of new[] and delete. In InnerList, scoped_ptr<char> was created with new[], but since without explicit destructor, it is destroyed with delete, which was the cause of the crash. Switched to char* and use delete[].
piman@chromium.org changed reviewers: + piman@chromium.org
https://codereview.chromium.org/400463002/diff/660001/cc/quads/list_container.cc File cc/quads/list_container.cc (right): https://codereview.chromium.org/400463002/diff/660001/cc/quads/list_container... cc/quads/list_container.cc:32: scoped_ptr<char> data; You can use scoped_ptr<char[]> instead.
https://codereview.chromium.org/400463002/diff/660001/cc/quads/list_container.cc File cc/quads/list_container.cc (right): https://codereview.chromium.org/400463002/diff/660001/cc/quads/list_container... cc/quads/list_container.cc:32: scoped_ptr<char> data; On 2014/09/23 02:32:14, piman (Very slow to review) wrote: > You can use scoped_ptr<char[]> instead. Thanks, didn't know that. Done.
lgtm https://codereview.chromium.org/400463002/diff/700001/cc/quads/list_container.cc File cc/quads/list_container.cc (right): https://codereview.chromium.org/400463002/diff/700001/cc/quads/list_container... cc/quads/list_container.cc:149: list->data = scoped_ptr<char[]>(new char[list->capacity * list->step]); nit: or .reset(new char[..])
https://codereview.chromium.org/400463002/diff/700001/cc/quads/list_container.cc File cc/quads/list_container.cc (right): https://codereview.chromium.org/400463002/diff/700001/cc/quads/list_container... cc/quads/list_container.cc:149: list->data = scoped_ptr<char[]>(new char[list->capacity * list->step]); On 2014/09/23 02:47:01, piman (Very slow to review) wrote: > nit: or .reset(new char[..]) Done. :)
The CQ bit was checked by weiliangc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/400463002/720001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
Message was sent while issue was closed.
Committed patchset #34 (id:720001) as 215126610b98166e27e87d84e65df9bd453805d2
Message was sent while issue was closed.
Patchset 34 (id:??) landed as https://crrev.com/d1f5016cb601daa67da3ca36eff3c8af1f35fa60 Cr-Commit-Position: refs/heads/master@{#296176} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
