API review

Proposer: Josh F

Present at review:

• Jack O'Quin
• Radu
• Stu
• Tully

Question / concerns / comments

Enter your thoughts on the API and any questions / concerns you have here. Please sign your name. Anything you want to address in the API review should be marked down here before the start of the meeting.

• (Jack) I am somewhat confused by the constructAll() method. I imagine it being used to preallocate objects in the pool. The API should explain the motivation explicitly.

  • [Josh] This is mainly meant for use by the ObjectPool class. I'll add a note of this.

• (Jack) Can the FreeList class keep track of whether constructAll() has been called and invoke destructAll() automatically in its destructor, if needed?

  • [Josh] Not without additional overhead that I don't think is warranted. Generally if you want this functionality you should use ObjectPool.

• (Radu) Suggestion: change allocate to allocateShared() and allocateBare() to allocate().

• (Radu) When I saw removeShared() I thought it meant something else. Is it doing just a .get()/*? Is it possible to rename it?

  • [Josh] removeShared() actually removes the shared_ptr (making the object no longer reference counted). This is only useful in very specific cases (such as rosrt's Subscriber).

• (Radu) Wondering if isUnallocated() can be changed (and negated) to isAllocated(), just to be a bit more similar to other libraries.

• (Stu) removeShared() is a confusing name. makeBare()? How attached are you to makeShared() and removeShared()? It feels like they just confuse the API without adding too much benefit.

  • [Josh] It's currently necessary for the rosrt's Subscriber.

• (Stu) Add an invocation of owns() that takes in a shared_ptr

• (Stu) isUnallocated is badly named. I'd prefer something plural, that indicates that all the blocks in the pool are deallocated. allDeallocated, hasOutstandingAllocations, hasActiveAllocations, allObjectsDeallocated, hasLiveAllocations, hasUsedObjects are all pretty bad too.

  • [Josh] Agreed, it was the best of all the bad options I came up with. I like hasOutstandingAllocations

• (Stu) FreeList should call destructAll automatically on destruction.

  • [Josh] Without additional overhead, it can't (it has no type information).

• (Stu) Extra awesome: FreeList::constructAll should verify that block_size can contain the object type.

• (Tully) If the allocator can fail if full, why isnt' the unAllocated() method concurrent safe?

Meeting agenda

To be filled out by proposer based on comments gathered during API review period

Conclusion

Package status change mark change manifest)

• Action items that need to be taken.

• Major issues that need to be resolvedo

• Add more documentation on the intended use of constructAll and destructAll vs. ObjectPool

• Change allocate to allocateShared and allocateBare to allocate

• Look into possibility of removing removeShared, but not for the first release.

• Change is Unallocated to hasOustandingAllocations

• Add invocation of owns() that takes in a shared_ptr

• constructAll will verify block_size