Re: [pygccxml-development] Order of enum values

[Matthias B. <ba...@ir...>]

Roman Yakovenko wrote:
>> Could the value actually also be something else than an int?
> 
> I am not sure. May be it possible to create an enum, with some 
> expression as
> value. And may be in this case gccxml will dump it as a string. I don't 
> know.
> So, small investigation is needed. If it could be expression, then
> 'int' in comment should be replaced to 'string'

I did some more reading and a couple of tests and it appears that it's 
always an int.

>> - Store the values inside a list instead of a dict. The items can be
>> either just the enum names or tuples (name, value) if the value is still
>> required somewhen.
> 
> You loose a convenience of dictionary: value = x[name].

Yes, but...

>> so maybe there are cases where a value has to be looked up quickly which
>> justifies using a dict.
> 
> There is no such place.

...as such a lookup is never used, we don't lose anything.

> If gccxml reports enums, using the order from source file, you can
> replace dict with
> a list, otherwise any order you define will surprise a user.

In my tests, gccxml always reported the enum values in the same order as 
they appear in the source file.

An enum like this

   enum Color { red=1, green=3, blue=2};

is reported in the order red, green, blue whereas it would be red, blue, 
green if sorted by numeric value.

> I prefer to make a change in pygccxml, but I am not sure, that it will
> be enough.
> Please take a look on decl_wrappers.enumeration_t. It also defines few
> containers,
> may be you will have to change those containers too. Please take a look 
> also
> on code_creators.enum_t and code_creators.unnamed_enum_t. They generate
> the code, I think you will have to fix them too :-(.
> 
> Please, implement what ever you think is right and I try to help you as 
> much as I can.

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))

- 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?)

- 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)

- Added a method has_value_name(). (This method would be more efficient 
with a dict but I didn't notice any slowdown so it's probably not worth 
spending any time on this. But it would be no problem adding an 
additional dict to the class)

=> The internal attribute _values is completely hidden by the class


scanner_t:
----------

- Converted the numeric value into an int

- Uses the append_value() method instead of directly manipulating class 
internals


parser.patcher.py:
------------------

- Updated __find_enum()
- Updated __is_unqualified_enum()


In pyplusplus:
==============

enumeration_wrapper.enumeration_t:
----------------------------------

- Added a property no_export_values that is the complementary of 
export_values.


code_creators.enum_t / code_creators.unnamed_enum_t:
----------------------------------------------------

- Updated code creation to reflect the changes of the enum declaration 
objects.

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.


I've tested the stuff with my Maya bindings and it seems to work fine 
(this will safe me quite some compilation time! :)
I've already committed the modifications so please have a look at them 
to make sure I didn't accidentally break anything else. Don't forget to 
remove any cache you have lying around (I stumbled across that one 
again! *sigh*)

Finally, I have some "criticism" towards the code (you should read this 
as suggestions for improvements in future code). 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.


- Matthias -