Re: [pygccxml-development] Order of enum values

[Roman Y. <rom...@gm...>]

On 7/8/06, Matthias Baas <ba...@ir...> wrote:
> I've replaced the dict in enumeration_t with a list and (hopefully)
> changed all other places that are affected by this. Here's what I changed:
>
> In pygccxml:
> ============
>
> enumeration_t:
> --------------
>
> - Store the values as an ordered list (same order as in the source
> file). List items are tuples (name(str), value(int))

I added new function called get_name2value_dict. There were too many places
in my unittests, where I needed it.

> - The 'values' property returns a *copy* of the internal list so that
> external code cannot mess with internal data.
> (Is that ok? Or is it intentional that a user receives the internal object?)

I don't have an opinion. In C++ I would return const reference to container,
but in Python ...? I prefere to think about declarations classes as
read only ones.
If user wants to modify them, I suppose he knows what he is doing.

> - Added a method append_value() that can be used to gradually construct
> the enum (instead of that the scanner has to know about internal details
> of the class)

Scanner and linker know almost all internal details of every class. In
this case,
it is possible to elimintate implementation details.

>
> It's not clear to me why the code creators also define properties
> value_aliases and export_values. I didn't use them in _create_impl() but
> I left them in anyway because I don't know if other code depends on them.

It is possible to rename an exported enum value

enum X{ x };

enum_<X>( "X" )
    .value( "XYZ", x );


> Finally, I have some "criticism" towards the code (you should read this
> as suggestions for improvements in future code).

I am always glad, when somebody does code review to what I wrote.

>  I think the
> enumeration_t class (and its wrapper class) wasn't properly
> encapsulating its data which I would regard as a design flaw. If they
> were encapsulating it, a modification like the above (changing internal
> data structures) should have been possible by only modifying
> enumeration_t itself. But other parts of the code were getting access to
> the internal dict and could do whatever they wanted with it without that
> the class has the chance to validate the data. That's why I have
> introduced new methods like append_value() which the scanner uses to add
> a new value to the enum without having to worry how exactly this has to
> be done (the enum class knows that better than the scanner). And for the
> same reason I modified the get/set methods for the 'values' property.
> Previously, they were passing the value right through to the internal
> attribute. This means a caller could obtain a reference to the internal
> dict and mess with it in any way. Or you could just set a value that
> contained corrupt data or that wasn't a dict at all. Now, retrieving the
> list gives you a copy so you can do with that copy what you want without
> accidentally corrupting the internal class data. When setting a new
> value some checks are done so that all in all, it's now much more
> difficult to get the enumeration class into an invalid state.
>
> This is not yet true for the enumeration wrapper class. This one still
> provides access to its internal _value_aliases and _export_values
> attributes. But I noticed in the unit tests that they're obviously
> supposed to be used like that so that you can add new values to those
> variables. I think a different way, such as using methods for adding
> values, where the class has the chance to validate the data would have
> been a more safe interface.
>
> As I said above, these were just some observations from me that I
> thought I should mention. I'm not expecting any immediate changes of the
> code (as long as it works it's ok), it's just supposed to be "food for
> thought" for future designs.

I understand, what you are talking about. Interface, that exposes functions,
is much better, stable and clear than interface built arround data structures
and that gives access to it's variables. I thought, that providing interface
built on "data structures" will simplify it. I will reconsider my approach.


Thank you very much for patch and very constructive "criticism".

-- 
Roman Yakovenko
C++ Python language binding
http://www.language-binding.net/