Re: [lmi] reusing mc_enum<> for serialization into XML

[Vaclav Slavik]

Hi,

Greg, could you have a look at this patch, to see if it's acceptable:
http://review.bakefile.org/r/154/diff/1/#index_header ?
(I added some inline comments and the highlighting helps a lot, that's
why I didn't include it in this mail this time -- let me know if it was
a mistake.) Raw patch is here:
http://review.bakefile.org/r/154/diff/raw/

I tried to keep the changes to minimum, so it doesn't fix all mc_emum<>
problems on your wish list, it only removes n,e,c template parameters.

On Sun, 2010-01-17 at 17:08 +0000, Greg Chicares wrote:
> Let me state my concerns and see what you think about them.
> 
> - This class is rather fundamental to lmi, and is used pervasively,
> so accidental pessimizations could be costly. That problem could be
> avoided by extensive testing. Aside from what 'mc_enum_test.cpp'
> covers, I'd be most concerned about bloating binary files and
> increasing the time or the amount of RAM required to build them.

I mostly only refactored the code, so I don't think I affected compiler
or runtime performance. I only make the assumption that the compiler
will inline simple functions such as this one:

    static enums_t enums() { return *e; }

If it does, then runtime performance should be exactly same.

> - I need to understand the resulting class (hierarchy) intimately.
> The more radical the changes, the harder it'd be for me to find
> enough time to review them promptly.

I avoided any changes to the way enum metadata are defined and checked.
This patch should be the smallest possible diff needed to remove n,e,c
template parameters.

> OTOH, there could be important advantages:
> 
> - Perhaps a refactored class could work around the known msvc
> defect without making everything really ugly.

I preserved your way of checking the bounds, I only moved it to another
class (mc_enum_type_info_impl<>). I still use the ugly msvc workaround
(by accident -- I branched off a version with this change already
applied and didn't realize it until much later), but it's now located in
one place only, the few lines of mc_enum_type_info_impl<> definition.
Hopefully that's localized enough to be OK for inclusion in the
mainline.

I hope this could be removed and replaced with something nicer, but I
didn't want to do it now, because it would make the changes too large.
So I introduced mc_enum_type_info_impl<> instead, in the interest of
simplicity.

> - This might be a nice way to get rid of extant ugliness like
>     std::string mc_str(mcenum_dbopt);

I didn't remove these yet, but will do if you think my approach in this
patch is agreeable.

> - Perhaps a faster alternative to std::find() could be used, so
> performance would improve--maybe even dramatically.

I didn't make any changes there, let's leave that for later.

> I should also briefly discuss what I don't necessarily like about
> this class, and therefore wouldn't insist on preserving. I'm not
> wedded to the use of arrays as non-type template parameters: it's
> a neat trick, but I've already derived quite enough satisfaction
> from using it and wouldn't insist on retaining it as a monument
> to "cleverness". 

I didn't change this, only moved it around.

> When adding a new type, I wanted to make it hard
> to make a mistake with array sizes--but I also made it rather hard
> to make any correct change. The metadata is scattered over too many
> files. 

I didn't fix this yet. At one point, it looked like I won't need
MC_DECLARE at all, but the way the tests are organized prevented me from
moving all of mc_enum_type_info<> metadata to mc_enum_types.cpp. I might
be able to find a way, but it enlarge the diff even more. 


BTW, a question inspired by Vadim: should we have some form of automated
tests to verify that too short or too long enum values/names list do in
fact break compilation?

Thanks,
Vaclav