Come by my office if you have any questions about anthing

REVIEW: New class interface and implementation makes sense. I see that any weirdness (like requiring users to sort peaks after adding one) simply replicates current Footprint behavior. I have a few questions just out of curiosity:

• Footprints have some functionality that Bootprints don’t: Merging, finding the distance, setting a mask. If this is going to be a drop in replacement, will that functionality be added? Or is it the responsibility of the contained SpanSet now and users will do: bootprint.getSpans().merge(… etc..?
• The commit says “Ultimately this commit will be erased in a rebase in leu of the final Footprint design.” Is this possible after it is merged to master? What are your plans for this ticket?
• Bootprint line 164: My understanding is that return std::move(..) should be avoided because it prevents Return Value Optimization (Meyers’ Effective Modern Item 25). Also, unique_ptr s are move-only anyway. What was the reason for casting as an rvalue via std::move before returning?

Comments

• Remember to add the copyleft to Bootprint.cc
• 215: I see why you were lamenting our lack of C++14 at lunch last week While not as much fun as if there were a make_unique available, this can be written without auto:
  std::unique_ptr<Bootprint> loadedBootprint(new Bootprint(loadedSpanSet));

• Bootprint.h: Given that it is a new class, documentation is especially important.

• • Line 50: Needs a class docstring. What is a Bootprint? Would you put everything you told me on Friday about it here: It contains a pointer to a SpanSet and PeakCatalog, and it is responsible for managing their relationship, e.g. checking that the peaks are contained by the SpanSet etc. This is also where you can say that it will replace Footprints in the future. And they are not heavy.
  • Line 55/56: Not sure why this comment is here or what it means.
  • Line 71: Returns -> Return
  • Line 94: Biref -> Brief
  • I could use some more words on what “dilate” and “erode” mean here. We use the word “grow” and “shrink” elsewhere in the stack. How is this different from growing and shrinking? It would be sufficient to include a reference to SpanSet’s docs.
  • Line 243: "peasks from the PeakCatlog" -> peaks from the PeakCatalog

Super small formatting stuff I wanted to draw your attention to:

Bootprint.h:

• line 67: Remove space after &&
• 246: Should be a space before “protected”
• 267: Not formatting, but leftover note to yourself

Bootprint.cc:

• 43/233/244: “val” could be more descriptive
• 63: There is an extra space in “bool doClip )”
• 109 - 171: Anonymous namespace contents are indented (this doesn’t bother me and DM style guide doesn’t have anything to say about it, but I don’t see any other code in the stack that does this)
• 161: 115 columns
• 113/132: “Private:” and “Public:” should be on same indent which is no indent .
• 158: Missing space before {
• 197-216 and 220-246: Code is currently indented by 7 spaces. Should be multiples of 4 .
• 236/237: Deprecated casting style. Use static_cast<int> )

Just to check — this is still marked as "in review". Are we expecting more to come, or is the review effectively completed?

To answer your questions:

• much of the functionality that is being removed will be either delegated to the SpanSet (combinations of direct method calls , (get|set)SpanSet calls, or free functions that are scheduled to be added in upcoming tickets.
• This will never get merged to master before the renaming. It is going to be on a new-footprints branch until 1. all the work is done 2. the pybind11 port is done 3. suitable notice can be sent out to everyone such that other code bases can be tested and people become familiar with the new code. I envision the renaming happening during the "make the stack work with the new footprints" ticket
• Basically in this case, read is an overloaded method of the inherited base class. The return type is declared to be a shared_ptr in the base class, but readSpanSet returns a unique_ptr (as in the rest of the code base we want footprints to be unique_ptrs) so in the return statement we must turn the unique_ptr back into an rvalue reference so that the compiler will implicitly convert it to the desired return type of shared pointer. I might change it so the creation of loadedBootprint is set to be a shared_ptr so the implicit conversion can happen there instead of in the return statement though.

comments:

• right
• Good point. I am too used to the other syntax from different work. I am not sure it makes a difference post compilation, but your suggestion is a good change for consistency.
• Initially I had another ticket I was going to worry about documentation on, but those got all refactored at some point, so you are right I should add it here.

The other points I will simply address before merging into the epic branch. Thank you for catching that formatting (though some of it is from copying older code) I kind of get blinded to formatting after staring at it for so long. I can't wait until clang format can take a lot of this away.

Also as a note, I will think if there is a better way to express the mathematical operations dilate and erode in the description, but these may be left as it. In the make the stack work will new footprints ticket, all references to grow and shrink will be removed. We are switching to these terms as these are the actual mathematical operations which are being evoked.

merged to epic branch 3559