Re: [PATCH 2/N] Add dump_flags_type<E> for handling of suboptions.

[Martin Sebor <msebor at gmail dot com>]

On 05/05/2017 04:44 AM, Martin Liška wrote:
> Hi.
>
> This one is more interesting as it implements hierarchical option parsing
> and as a first step I implemented that for optgroup suboptions.

I haven't gone through the rest of the patches so I could be
missing some context.  But I have a few observations about and
suggestions for the new dump_option_node (and to a smaller extent,
dump_flags_type) class template. (Feel free to disregard whatever
doesn't make sense.)

First, since dump_option_node's public member functions (including
the only ctor) are defined in dumpfile.c it seems that the template
definition doesn't really need to be provided in the header. If
that's correct, it would reduce coupling and might avoid bloat to
only declare the bare minimum needed to use the type in other .c
files, and define the rest in the one .c file where the complete
type is actually needed.

As far as I can see, there is just one instantiation of the template
in the patch, in dumpfile.c:

+  typedef dump_option_node<optgroup_types> node;

If there are no other existing instantiations and no others are
anticipated, it could even be a plain class (further reducing
complexity and bloat).

Finally, the template ctor takes a const char* and stores it
in a member, and the implicit copy ctor and assignment operator
copy the underlying vec class.  That means that the string
argument must outlive the constructed object, which is not
typically expected.  IIUC, vec is not safely copy-assignable
or copy-constructible (i.e., has undefined behavior).  At
a minimum, it would be appropriate to document these
constraints.  Removing them or preventing copy construction
and assignment to avoid getting bit by it would be even better.

For dump_flags_type, none of the members need to be explicitly
declared inline or mention the <E> template argument list.
operator&(dump_flags_type) should be declared const.  Compound
assignment operators should return a reference to *this  (to
behave as closely to the native operators).  The exclusive OR
operator and compound assignment are missing and should probably
be provided for completeness, even if they aren't needed.  The
m_mask member should probably be private for better encapsulation
(and operator uint64_t() const provided to provide implicit
conversion to that type, or perhaps one converting to E's
underlying type; that should also obviate the need for operator
bool).

Martin