Re: [patch 3/N] std::regex refactoring - _Executor DFS / BFS

[Tim Shen <timshen91 at gmail dot com>]

On Mon, Apr 28, 2014 at 10:46 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
> This change splits _Executor::_M_main() into two overloaded
> _M_main_dispatch() functions, choosing which to run based on the
> __dfs_mode template parameter.
>
> I think this gives a (very) small improvement in compilation time when
> using regexes.
>
> Splitting _M_main() allows the _M_match_queue and _M_visited members
> (which are only used in BFS mode) to be replaced with an instantiation
> of the new _States class template. _States<__dfs> only contains the
> start state (so that it's not empty and doesn't waste space) but
> _States<__bfs> contains the start state and also the match queue and
> visited list.

This is great! I keep worrying about the ugly all-in-one _Executor
class; I've tried to specialize two versions of it, but that
introduced much duplicated code. Your abstract is nice!

> As the visited list never changes size after construction I changed
> its type from unique_ptr<vector<bool>> to unique_ptr<bool[]>, which
> avoids an indirection, although now that that member is only present
> when actually required it could just be vector<bool>. An array of bool
> is simpler to access, but takes more heap memory. vector<bool> uses
> less space but the compiler has to do all the masking to address
> individual bits.

What about bitset? Anyway we should get rid of vector<bool>.

> I also changed this range-based for loop in _M_main (now in
> _M_main_dispatch(__bfs)) to make __task a reference, avoiding one
> copy, and to move from __task.second, avoiding a second copy:
>
>        auto __old_queue = std::move(_M_states._M_match_queue);
>        for (auto& __task : __old_queue)
>          {
>            _M_cur_results = std::move(__task.second);
>            _M_dfs<__match_mode>(__task.first);
>          }

I didn't know why I've written the code that copys a lot. Thanks.


-- 
Regards,
Tim Shen