This is an archive of the discontinued LLVM Phabricator instance.

Implement LFTS searchers. Boyer_Moore and Boyer_Moore_Horspool
ClosedPublic

Authored by mclow.lists on Jul 20 2015, 7:23 PM.

Details

Diff Detail

Event Timeline

mclow.lists retitled this revision from to Implement LFTS searchers. Boyer_Moore and Boyer_Moore_Horspool.
mclow.lists updated this object.
mclow.lists added reviewers: EricWF, howard.hinnant.
mclow.lists added a subscriber: cfe-commits.

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.

EricWF edited edge metadata.Jul 28 2015, 3:57 PM

drive-by comments.

include/experimental/functional
158

Do we want to copy the __val here?

160

Would skip_.insert(val) be better here?

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.

mclow.lists added inline comments.Jul 28 2015, 10:15 PM
include/experimental/functional
158

See line #192.

191

value_type is a difference_type. (size_t, usually).

214

Also unsigned char, signed char, uint8_t, int8_t (and maybe enums).

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

  1. Use less memory at any given time.
  2. Reuse previously allocated memory.

I think the following changes could improve the QoI.

  1. Move the loop that uses __prefix to be directly following the initialization of it.
  2. Pass reverse_iterator adaptors to __compute_bm_prefix instead of computing reversed.
  3. Reuse the memory in __prefix for __prefix_reversed.

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.

Two comments answered. One still remaining.

include/experimental/functional
255

Well, for debugging. My first comment addressed this (basically, it will get changed).

312

You are correct.

mclow.lists updated this revision to Diff 33770.Sep 1 2015, 5:36 PM
mclow.lists edited edge metadata.

Updated based on Eric's comments.

mclow.lists marked 3 inline comments as done.Sep 1 2015, 5:36 PM
EricWF accepted this revision.Sep 4 2015, 2:23 PM
EricWF edited edge metadata.

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.

This revision is now accepted and ready to land.Sep 4 2015, 2:23 PM
mclow.lists closed this revision.Sep 8 2015, 11:01 AM

Landed as revision 247036