Details
- Reviewers
EricWF howard.hinnant
Diff Detail
Event Timeline
Just noticed that I left some TODOs in the patch; places where I replaced private with public for debugging purposes. I'll fix that before committing.
drive-by comments.
include/experimental/functional | ||
---|---|---|
158 | Do we want to copy the __val here? | |
160 |
I don't think so but it depends on the semantics of this insert method. Should existing values be overwritten? | |
180 | std::numeric_limits<unsigned_key_type>::max() might be clearer. It took me a while to figure out what this was doing. | |
191 | Do we want the extra copy of __val here? | |
214 | Is this only meant to trigger for char and bool? I think it would be wrong to use the array specialization for bool because it can only represent 2 values but the array will end up having a size of 256. |
Added more review comments for the boyer_moore searcher.
include/experimental/functional | ||
---|---|---|
255 | Is this for testing? | |
312 | This needs to be a reserved identifier right? | |
318 | It seems like we could do a lot more in this section to
I think the following changes could improve the QoI.
Unless I'm missing something those changes should be possible, and they reduce the amount of memory used by two thirds. |
@mclow.lists I'm holding off on more review until the current comments are addressed.
LGTM modulo handling C++03. Currently <experimental/functional> compiles w/ clang in C++03. Please either ensure your portions of the searchers also compile in C++03 and change the tests or #ifdef them out before committing.
Do we want to copy the __val here?