Container operations - Githubissues

GrammaTech / gtirb

Intermediate Representation for Binary analysis and transformation

https://grammatech.github.io/gtirb/

Other

305 stars 36 forks source link

Container operations #23

Closed nweston closed 5 years ago

nweston commented 5 years ago

Ensure that all containers support the lookup operations we need with good efficiency:

DataObjects are now indexed by address
Sections are now indexed by address
Symbols are indexed by name and address
Symbol address is now optional
Blocks now store a vertex_descriptor for fast lookup into the CFG
Added find methods to Module corresponding to all the new indices

We had discussed the use of boost::multi_index for symbols. However, our limited API makes it easy to maintain two separate maps for symbols, and since we're hiding the details of the container there is no additional benefit to using multi_index.

AaronBallman commented 5 years ago

Why did you merge this to master before getting review sign-off?

nweston commented 5 years ago

We discussed and approved it in the meeting yesterday. Since you're out of office through the planned release, we weren't expecting you to follow up on code reviews (which is why I assigned this to Tom in the first place).

AaronBallman commented 5 years ago

We discussed and approved it in the meeting yesterday. Since you're out of office through the planned release, we weren't expecting you to follow up on code reviews (which is why I assigned this to Tom in the first place).

Thanks for the info. I think our practices should be: 1) sign offs happen in the review, for tracking and transparency purposes, and 2) once someone starts commenting on a review, they should sign off on the review before it gets merged.

I was in the middle of the review and had open questions and further review feedback. I don't think it's a huge deal in this case because nothing was fundamental, though.