After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 86864 - enums should be inside classes.
enums should be inside classes.
Status: RESOLVED FIXED
Product: glibmm
Classification: Bindings
Component: general
2.17.x
Other Linux
: Normal normal
: ---
Assigned To: gtkmm-forge
gtkmm-forge
Depends on:
Blocks:
 
 
Reported: 2002-06-29 19:29 UTC by Murray Cumming
Modified: 2017-06-11 11:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix (801 bytes, patch)
2003-03-12 20:46 UTC, Sebastian Rittau
none Details | Review
patch: gmmproc: _WRAP_ENUM can be inside classes (15.95 KB, patch)
2011-03-03 15:59 UTC, Kjell Ahlstedt
none Details | Review
Archive file with modified expander.hg and filechooser.hg (19.15 KB, application/x-compressed-tar)
2011-03-16 15:04 UTC, Kjell Ahlstedt
  Details
patch: gmmproc: Part 2 of _WRAP_ENUM inside classes. (9.16 KB, patch)
2011-03-31 17:02 UTC, Kjell Ahlstedt
none Details | Review
0001-SizeGroup-Rename-SizeGroupMode-enum-to-SizeGroup-Mod.patch (5.34 KB, patch)
2017-04-17 06:52 UTC, Murray Cumming
none Details | Review
0001-Gdk-Window-Change-WindowTypeHint-to-Window-TypeHint.patch (3.82 KB, patch)
2017-04-27 15:58 UTC, Murray Cumming
none Details | Review
0001-GutterRenderer-Move-enums-into-the-class.patch (4.97 KB, patch)
2017-04-27 20:53 UTC, Murray Cumming
none Details | Review
patch: gmmproc, _WRAP_ENUM: Add optional CONV_TO_INT parameter (13.00 KB, patch)
2017-05-12 14:49 UTC, Kjell Ahlstedt
committed Details | Review

Description Murray Cumming 2002-06-29 19:29:08 UTC
Some enums should be inside other classes, rather than part of the whole
Gtk:: namespace. I tried this with AtkRelationType:


class Relation : public Glib::Object
{
   _CLASS_GOBJECT(Relation, AtkRelation, ATK_RELATION, Glib::Object, GObject)

  _WRAP_ENUM(Type, AtkRelationType)


but I get this compiler error:

n file included from ../../atk/atkmm/relationset.h:30,
                 from object.cc:30:
../../atk/atkmm/relation.h:35: conflicting types for `typedef struct 
_AtkRelationType AtkRelationType'
/gnome/head/INSTALL/include/atk-1.0/atk/atkrelation.h:58: previous
declaration as `typedef enum 
AtkRelationType AtkRelationType'

For some reason it thinks that AtkRelationType is a struct.
Comment 1 Murray Cumming 2002-07-04 17:28:45 UTC
Partially fixed, by using different variable names in enums.m4, but we
still need some way to place the enum's operator overloads outside of
the class, while having the enums inside the class.
Comment 2 Murray Cumming 2002-07-26 15:29:58 UTC
Punting, because it's difficult to implement.
Comment 3 Murray Cumming 2003-01-28 13:06:44 UTC
If someone can provide some example C++ code (maybe a patch of 
generated .h/.cc files) then we might be able to implement this.
Comment 4 Sebastian Rittau 2003-03-12 20:46:37 UTC
Created attachment 14975 [details] [review]
Fix
Comment 5 Sebastian Rittau 2003-03-12 20:47:22 UTC
Forget the attachment. Bugzilla drives me nuts!
Comment 6 Murray Cumming 2003-03-13 07:11:25 UTC
(the patch was for a different bug)
Comment 7 Murray Cumming 2004-02-11 18:40:24 UTC
Nathan Myers said this in a direct email to me (Dont' do that - use
bugzilla or the mailing list):

You said in 86864, "we still need some way to place the enum's
operator overloads outside of the class, while having the enums inside
the class".I don't know of any difficulty in that, unless the enum
type weredeclared private.  You can define operator overloads on any
enum type, member or otherwise.  The only problem is that the
one-definition rule may be hard to satisfy; i.e. since anybody can
define overloads,somebody else might, and you have nothing stronger
than documentation to keep them from doing it.  In practice, that's
probably not a problem for your case.
Comment 8 Kjell Ahlstedt 2011-03-03 15:59:56 UTC
Created attachment 182357 [details] [review]
patch: gmmproc: _WRAP_ENUM can be inside classes

This patch makes it possible to put _WRAP_ENUM inside classes in most cases.
The possibilities and limitations are described in a comment in WrapParser.pm.

# _WRAP_ENUM(cpp_type, c_type [,NO_GTYPE] [,NO_OPERATORS] [,s#regexpr#subst#]* [,get_type_func=])
# Optional arguments:
# NO_GTYPE      Don't generate code for a specialization of the template
#               Glib::Value_Enum or Glib::Value_Flags.
#               Necessary, if the C type enum is not registered as a GType.
# NO_OPERATORS  Don't generate code for bitwise operators. This is the default
#               if the enum is not a Flags type.
# s#regexpr#subst#  Zero or more substitutions in names of enum constants, e.g. s#^DATE_##.
# get_type_func=  Ignored.
#
# _WRAP_ENUM can be located either in a class or outside all classes.
# When located in a class, and Value specialization and/or bitwise operators
# shall be generated, then the following requirements must be fulfilled:
# 1. _WRAP_ENUM must be located in the public part of the class.
# 2. The class must contain a class macro (_CLASS_GENERIC, _CLASS_GOBJECT,
#    _CLASS_GTKOBJECT, etc.) before _WRAP_ENUM.

It would be possible to remove requirement 2, but is it worth the trouble? Very
few classes lack a _CLASS_xxx macro.

I've tested my changes by generating all code for atkmm, glibmm and gtkmm, and
compared with code generated before my changes. The only differences are that
some blank lines have disappeared, and that the indentation is different in
a few lines in glibmm/checksum.h. Slight improvements, in my opinion.

This is the first time I've modified an M4 file, and now I appreciate
Krzesimir Nowak's plan to replace M4 in gmmproc by Perl code in the future
(bug 641165 comment 3).
Comment 9 Murray Cumming 2011-03-15 10:02:03 UTC
Oh, thanks.

Can you show how this might be used in gtkmm. For instance, to change 
Gtk::FileChooserAction with Gtk::FILE_CHOOSER_ACTION_OPEN 
  http://library.gnome.org/devel/gtkmm/unstable/group__gtkmmEnums.html#ga0d6076e7637ec501f26296e65fee2212
to Gtk::FileChooser::Action with Gtk::FileChooser::ACTION_OPEN

There are also some enum values that we would need to change to add a common prefix. For instance, changing Gtk::ExpanderStyle with Gtk::EXPANDER_COLLAPSED 
  http://library.gnome.org/devel/gtkmm/unstable/group__gtkmmEnums.html#ga89d4d4f59bc9ab0c75c48f24c512ed1c
to Gtk::Expander::Style, with Gtk::Expander::STYLE_COLLAPSED
Comment 10 Kjell Ahlstedt 2011-03-16 15:04:14 UTC
Created attachment 183545 [details]
Archive file with modified expander.hg and filechooser.hg

The attached archive shows one way to do it. It contains the files
  expander.hg, .h, .cc
  filechooser.hg, .h, .cc
  new_conversions.m4  Some M4 macros that I had to add.

I also had to modify FileChooser's subclasses, but I've not included those
modifications in the attachment.

I noticed that the patch in comment 8 is incomplete in at least two ways:

1.
More conversion macros are needed. This is probably easy to fix.

2.
The conversion of identifier names from glib/gtk to glibmm/gtkmm in
glibmm/tools/pm/DocsParser.pm does not correctly convert enum type names and
enum constant names.
This is more problematic. How can substitute_identifiers() in DocsParser.pm
know if GtkFileChooserAction and GTK_FILE_CHOOSER_ACTION_OPEN shall be
converted to Gtk::FileChooserAction and Gtk::FILE_CHOOSER_ACTION_OPEN or to
Gtk::FileChooser::Action and Gtk::FileChooser::ACTION_OPEN?
Of course it's possible to copy descriptions to xxx_docs_override.xml and
change them manually, but that's not what you'd like to do.
Comment 11 Kjell Ahlstedt 2011-03-21 10:24:52 UTC
(In reply to comment #10)
> More conversion macros are needed.

I think two new conversion macros should be added, if the patch in comment 8 is
committed.
- In glibmm/tools/m4/convert_base.m4: _CONV_CLASS_ENUM, as in the attachment
  in comment 10.
- In glibmm/tools/m4/convert_glib.m4: _CONV_GLIB_CLASS_ENUM.

I can make a patch, but I'll wait until I know if the patch in comment 8 is
accepted as is, or if I need to modify it.

> The conversion of identifier names from glib/gtk to glibmm/gtkmm in
> glibmm/tools/pm/DocsParser.pm does not correctly convert enum type names and
> enum constant names.

See e.g. the description of Gtk::FileChooser::set_action(FileChooser::Action
action) in the attachment in comment 10, where %GTK_FILE_CHOOSER_ACTION_SAVE in
the C documentation is changed to Gtk::FILE_CHOOSER_ACTION_SAVE. (Shall be Gtk::FileChooser::ACTION_SAVE.)

DocsParser.pm probably needs a manually written translation table, either in
xxx_docs_override.xml or in a separate file. Such a table could be useful for
other translations as well. Look e.g. at the hash table %gtk_objects in
DocsParser::non_object_method_name(). Wouldn't it be better to read that table
from gtkmm/gtk/src/gtk_docs_override.xml?

I welcome comments from people with more experience with gmmproc. Am I on the
right track, or is there a better way to improve the documentation of the C++
functions?
Comment 12 Kjell Ahlstedt 2011-03-31 17:02:19 UTC
Created attachment 184802 [details] [review]
patch: gmmproc: Part 2 of _WRAP_ENUM inside classes.

The patch contains a suggested fix for the issues discussed in comment 11.

In comment 11 I mentioned DocsParser::non_object_method_name(). The patch
removes that function. I discovered that it's unnecessary because of gtkmm/gtk/
src/gtk_extra_objects.defs. All it does is a few erroneous translations of the
function names gtk_drag_source_*. Without DocsParser::non_object_method_name()
those names are not translated. The C names are kept in the C++ documentation.
That's not perfect of course, but it's no worse than translating them to
something that looks like the names of C++ methods, but no methods with those
names exist. The functions gtk_drag_source_* and gtk_drag_dest_* don't fit
into the naming convention, since they are wrapped into Gtk::Widget.
Comment 13 Kjell Ahlstedt 2012-05-08 07:29:13 UTC
Does C++11 make this bug obsolete, or at least less urgent?
(Considering it was filed 10 years ago and has not been fixed yet, it has
probably never been urgent.)

I can think of two reasons for defining an enum inside a class.

1. Avoid polluting the surrounding namespace with all the enum constants.

2. The enum type is closely related to the class, and has little or no use
   except in calls to the class methods.

C++11 offers another solution to the namespace pollution problem. Instead of
defining

  namespace Gtk
  {
    enum FileChooserAction
    {
      FILE_CHOOSER_ACTION_OPEN,
      FILE_CHOOSER_ACTION_SAVE,
      FILE_CHOOSER_ACTION_SELECT_FOLDER,
      FILE_CHOOSER_ACTION_CREATE_FOLDER
    };
  }
  Gtk::FileChooserAction action = Gtk::FILE_CHOOSER_ACTION_SAVE;
  
you can define

  namespace Gtk
  {
    enum class FileChooserAction // Note 'enum class'
    {
      OPEN,
      SAVE,
      SELECT_FOLDER,
      CREATE_FOLDER
    };
  }
  Gtk::FileChooserAction action = Gtk::FileChooserAction::SAVE;
Comment 14 Murray Cumming 2014-02-17 08:27:52 UTC
(In reply to comment #13)
> 1. Avoid polluting the surrounding namespace with all the enum constants.
> 
> 2. The enum type is closely related to the class, and has little or no use
>    except in calls to the class methods.

Yes, that's the idea.

> C++11 offers another solution to the namespace pollution problem.
[snip]
>   Gtk::FileChooserAction action = Gtk::FileChooserAction::SAVE;

Yes, that seems slightly nicer than the current enums, but that seems like a separate issue. And use of C++11 is dependent on C++11 support being officially stable in g++, but I have no idea whatsoever when that might happen.
Comment 15 Murray Cumming 2014-02-17 08:29:21 UTC
Let's try to use these patches if/when we do an ABI break. GTK+ 4 would be a good time for that, if it happens, though that's far from clear.
Comment 16 Kjell Ahlstedt 2014-02-17 14:48:02 UTC
The patches in comments 8 and 12 only affect gmmproc. They (or updated
versions of them) can be applied any time without breaking ABI or API.
But unless ABI and API can be broken, the new functionality can be used only
for new enums. New enums are introduced very seldom. I suppose that's why you
suggest we wait until GTK+ 4. I think that's a good choice. Having unused
functionality in gmmproc for several years is not very good.
Comment 17 Kjell Ahlstedt 2017-03-29 13:48:23 UTC
(In reply to Murray Cumming from comment #15)
> Let's try to use these patches if/when we do an ABI break. GTK+ 4 would be a
> good time for that.

Gtk+ 4 is almost here now. Is this ancient bug worth considering? Or isn't it
worth the trouble? Close with Wontfix? The need for _WRAP_ENUM within classes
obviously has never been high. Or else this bug would have been fixed a long
time ago.
Comment 18 Murray Cumming 2017-03-30 10:03:18 UTC
(In reply to Kjell Ahlstedt from comment #17)
> (In reply to Murray Cumming from comment #15)
> > Let's try to use these patches if/when we do an ABI break. GTK+ 4 would be a
> > good time for that.
> 
> Gtk+ 4 is almost here now.

I am not particularly confident that GTK+ 4 will be stable, or even much used, any time particularly soon.

> Is this ancient bug worth considering? Or isn't it
> worth the trouble? Close with Wontfix? The need for _WRAP_ENUM within classes
> obviously has never been high. Or else this bug would have been fixed a long
> time ago.

I still think it would be nice.
Comment 19 Kjell Ahlstedt 2017-03-30 14:51:34 UTC
Now that we can break API and ABI it's tempting to let _WRAP_ENUM and
_WRAP_GERROR generate enum class instead of plain enum.

With GtkFileChooserAction as an example of enum class:

- _WRAP_ENUM(FileChooserAction, GtkFileChooserAction) outside class FileChooser:
  GtkFileChooserAction         --> Gtk::FileChooserAction
  GTK_FILE_CHOOSER_ACTION_OPEN --> Gtk::FileChooserAction::OPEN
  With plain enum:                 Gtk::FILE_CHOOSER_ACTION_OPEN

- _WRAP_ENUM(Action, GtkFileChooserAction, s#^FILE_CHOOSER_##) in the class:
  GtkFileChooserAction         --> Gtk::FileChooser::Action
  GTK_FILE_CHOOSER_ACTION_OPEN --> Gtk::FileChooser::Action::OPEN
  With plain enum:                 Gtk::FileChooser::ACTION_OPEN

All uses of enum constants would have to be changed.
Comment 20 Murray Cumming 2017-03-30 19:20:11 UTC
Yes, I'd like that too.
Comment 21 Kjell Ahlstedt 2017-04-06 09:25:23 UTC
In case someone's wondering: I'm working with this. Glibmm is almost finished,
but I don't want to push it yet. The change from plain enum to enum class
requires lots of changes. I'd like to fix pangomm, atkmm and gtkmm before I
push anything. Otherwise those modules won't build with the latest glibmm for
some time, perhaps one or two weeks.

I have not modified _WRAP_GERROR. I don't think it would benefit from using
enum class. Each enum is defined within its own class. enum class would require
e.g. that Glib::OptionError::UNKNOWN_OPTION is replaced by
Glib::OptionError::Code::UNKNOWN_OPTION. That's no improvement.
Comment 22 Kjell Ahlstedt 2017-04-11 18:10:53 UTC
I have pushed a glibmm patch that that modifies _WRAP_ENUM in two ways:
https://git.gnome.org/browse/glibmm/commit/?id=77c00208d07a74f4fdf185feb90017ae959f1763

1. It is expanded to an enum class instead of a plain enum. It means that
it can't be implicitly converted to an int, and that enumerator names are
local to the enum.

2. It can be located in a class, with some restrictions.
When located in a class, and a Value specialization shall be generated
(NO_GTYPE is not specified) or it's a Flags type (i.e. bitwise operators
shall be generated), then the following requirements must be met:
2.1. _WRAP_ENUM must be located in the public part of the class.
2.2. The class must contain a class macro (_CLASS_GENERIC, _CLASS_GOBJECT,
     _CLASS_GTKOBJECT, etc.) before _WRAP_ENUM.

The shift from plain enum to enum class requires lots of changes both in
glibmm and in other modules. I have pushed the necessary changes in glibmm,
atkmm, pangomm, and gtkmm. I'll fix gtkmm-documentation later. I can fix a few
other modules, too (at least if they are smaller than gtkmm), but I'll do that
only on request.

It remains to actually move some _WRAP_ENUMs inside classes. Suggestions which
ones to move?
Comment 23 Murray Cumming 2017-04-15 12:20:49 UTC
How about Gtk::MessageDialog::MessageType (or Type), Gtk::Dialog::ResponseType (or Response), Gtk::FileChooser::Action?
Comment 24 Murray Cumming 2017-04-15 16:40:11 UTC
Also, Gio::ApplicationFlags, Gio::FileType, Gtk::ButtonBoxStyle, Gtk::SizeGroupMode, Gtk::PrintOperationAction, Gdk::CursorType.

Maybe ShadowType and PolicyType in ScrolledWindow.

Maybe EventMask in Gdk::Event.


By the way, many thanks for this. It revealed a few actual errors in my code, where I had passed an enum to a parameter expecting a bool, for instance when calling the Gtk::MessageDialog constructor.


Some types are meant to be used as ints (Gtk::ResponseType) or guint (GtkBuiltinStockSize), so we now need to cast the enum values. Maybe we can add something to make those conversions happen implicitly. Maybe just a standalone operator int(), for instance.
Comment 25 Kjell Ahlstedt 2017-04-16 09:16:14 UTC
> Some types are meant to be used as ints (Gtk::ResponseType) or guint
> (GtkBuiltinStockSize), so we now need to cast the enum values. Maybe we can
> add something to make those conversions happen implicitly. Maybe just a
> standalone operator int(), for instance.

GtkBuiltinStockSize? Do you mean Gtk::BuiltinIconSize?

Yes, I've noticed that it would be fine with an implicit conversion from
Gtk::ResponseType to int, and also from Pango::Weight to int.
I've partly solved the problem for Gtk::ResponseType by defining
operator==(int, Gtk::ResponseType) and three similar operators, plus
class Gtk::Dialog::ResponseTypeOrInt.

It's not possible to define a standalone assignment operator,
  operator=(int lhs, Gtk::ResponseType rhs);
User-defined assignment operators must be class members.

Is a standalone operator int() possible? When I try
  operator int(ResponseType rhs) { return static_cast<int>(rhs); }
g++ says "error: ‘Gtk::operator int(Gtk::ResponseType)’ must be a nonstatic
member function".
Comment 26 Murray Cumming 2017-04-16 11:34:55 UTC
(In reply to Kjell Ahlstedt from comment #25)
> > Some types are meant to be used as ints (Gtk::ResponseType) or guint
> > (GtkBuiltinStockSize), so we now need to cast the enum values. Maybe we can
> > add something to make those conversions happen implicitly. Maybe just a
> > standalone operator int(), for instance.
> 
> GtkBuiltinStockSize? Do you mean Gtk::BuiltinIconSize?

Yes, sorry.

> Yes, I've noticed that it would be fine with an implicit conversion from
> Gtk::ResponseType to int, and also from Pango::Weight to int.
> I've partly solved the problem for Gtk::ResponseType by defining
> operator==(int, Gtk::ResponseType) and three similar operators, plus
> class Gtk::Dialog::ResponseTypeOrInt.

Interesting. Could I see that patch, please?

> It's not possible to define a standalone assignment operator,
>   operator=(int lhs, Gtk::ResponseType rhs);
> User-defined assignment operators must be class members.
> 
> Is a standalone operator int() possible? When I try
>   operator int(ResponseType rhs) { return static_cast<int>(rhs); }
> g++ says "error: ‘Gtk::operator int(Gtk::ResponseType)’ must be a nonstatic
> member function".

This might just not be possible. Maybe, at least with C++11, we are just not supposed to use enums for int constants. Maybe we should really be using this sometimes instead:

class ResponseType {
  constexpr int NONE = -1;
  constexpr int REJECT = -2;
  constexpr int ACCEPT = -3;
  ...
};

That gives us the ResponseType::NONE syntax that we want with the C++11 class enums, without the strong type-safety that they enforce.
Comment 27 Murray Cumming 2017-04-17 06:52:48 UTC
Created attachment 349931 [details] [review]
0001-SizeGroup-Rename-SizeGroupMode-enum-to-SizeGroup-Mod.patch

Does this seem OK, for instance?
Comment 28 Kjell Ahlstedt 2017-04-17 13:34:22 UTC
> Interesting. Could I see that patch, please?

It's included in the large patch https://git.gnome.org/browse/gtkmm/commit/?id=e6e4de8bcd69e92a4836956e6fdfc66b54c6174c
"Fix build when _WRAP_ENUM generates enum class".
See the modification of gtk/src/dialog.hg in that patch.
It does not allow implicit conversion of ResponseType to int in all situations,
for instance not in a switch statement, like in https://git.gnome.org/browse/gtkmm-documentation/tree/examples/book/dialogs/messagedialog/examplewindow.cc

> class ResponseType {
>   constexpr int NONE = -1;
>   constexpr int REJECT = -2;
>   constexpr int ACCEPT = -3;
>   ...
> };

Or perhaps something like
  class ResponseType {
  public:
    enum Enum {
      NONE = -1,
      REJECT = -2,
      ACCEPT = -3,
      ...
    };
  };
I guess that the necessary fix of _WRAP_ENUM would be easier, because the
generated code would be more like what _WRAP_ENUM and WRAP_GERROR generate now.

> Does this seem OK, for instance?

Your SizeGroupMode patch looks OK, except that convert_gtk.m4 should be
-_CONV_ENUM(Gtk,SizeGroupMode)
+_CONV_INCLASS_ENUM(Gtk,SizeGroup,Mode)

I added _CONV_INCLASS_ENUM to glibmm/tools/m4/convert_base.m4. It's better than
_CONV_ENUM for enums in classes. I also added _CONV_GLIB_INCLASS_ENUM to
convert_glib.m4.
Comment 29 Murray Cumming 2017-04-18 08:54:31 UTC
> > class ResponseType {
> >   constexpr int NONE = -1;
> >   constexpr int REJECT = -2;
> >   constexpr int ACCEPT = -3;
> >   ...
> > };
> 
> Or perhaps something like
>   class ResponseType {
>   public:
>     enum Enum {
>       NONE = -1,
>       REJECT = -2,
>       ACCEPT = -3,
>       ...
>     };
>   };

What's the advantage of using and (old-style) enum there? If we do use an old-style enum, I don't think the "Enum" name is necessary - the enum can be unnamed.

> I guess that the necessary fix of _WRAP_ENUM would be easier, because the
> generated code would be more like what _WRAP_ENUM and WRAP_GERROR generate
> now.

Sorry, I don't understand what you are suggesting in that last sentence.

> > Does this seem OK, for instance?
> 
> Your SizeGroupMode patch looks OK, except that convert_gtk.m4 should be
> -_CONV_ENUM(Gtk,SizeGroupMode)
> +_CONV_INCLASS_ENUM(Gtk,SizeGroup,Mode)

Thanks. I have made that change and pushed this. I might make a few more similar changes.
Comment 30 Murray Cumming 2017-04-18 09:15:53 UTC
> I also added _CONV_GLIB_INCLASS_ENUM to convert_glib.m4.

Do we need something similar for convert_gio.m4?
Comment 31 Kjell Ahlstedt 2017-04-18 18:27:39 UTC
>> I also added _CONV_GLIB_INCLASS_ENUM to convert_glib.m4.
>
> Do we need something similar for convert_gio.m4?

I have added _CONV_GIO_ENUM, _CONV_GIO_DBUS_ENUM, _CONV_GIO_INCLASS_ENUM and
_CONV_GIO_DBUS_INCLASS_ENUM. https://git.gnome.org/browse/glibmm/commit/?id=c2eb712b177f0e6ac31beb1de0288d25532b9f45

> What's the advantage of using and (old-style) enum there? If we do use an
> old-style enum, I don't think the "Enum" name is necessary - the enum can be
> unnamed.

Yes, an unnamed enum is better here.
The only advantage with an old-style enum instead of constexpr constants is that
I think that more of the perl and m4 code for _WRAP_ENUM can be reused. But so
far it's just a guess. It requires a new parameter in _WRAP_ENUM. How about
  _WRAP_ENUM(ResponseType, GtkResponseType, CONVERTIBLE_TO_INT))?
Comment 32 Kjell Ahlstedt 2017-04-19 07:12:22 UTC
> Yes, an unnamed enum is better here.

Probable the enum must have a name, after all. If NO_GTYPE is not specified in
_WRAP_ENUM, a Glib::Value<> specialization is generated for the enum. I don't
think that's possible if the enum does not have a name. And if it's a Flags type
_WRAP_ENUM generates bitwise operators. Impossible for a type without a name,
I suppose.
Comment 33 Murray Cumming 2017-04-19 08:33:31 UTC
(In reply to Kjell Ahlstedt from comment #31)
> The only advantage with an old-style enum instead of constexpr constants is
> that
> I think that more of the perl and m4 code for _WRAP_ENUM can be reused.

I'd prefer if it generated the best code, if possible.

I will post to the mailing list to see if there are strong opinions about how to group sets of constants in "modern C++".
Comment 34 Murray Cumming 2017-04-19 08:35:01 UTC
(In reply to Kjell Ahlstedt from comment #32)
> > Yes, an unnamed enum is better here.
> 
> Probable the enum must have a name, after all. If NO_GTYPE is not specified
> in
> _WRAP_ENUM, a Glib::Value<> specialization is generated for the enum. I don't
> think that's possible if the enum does not have a name. And if it's a Flags
> type
> _WRAP_ENUM generates bitwise operators. Impossible for a type without a name,
> I suppose.

Do any of these "sets of constants" enums need a Glib::Value<> specialization or are any of them used as flags?
Comment 35 Murray Cumming 2017-04-27 15:58:16 UTC
Created attachment 350561 [details] [review]
0001-Gdk-Window-Change-WindowTypeHint-to-Window-TypeHint.patch

Is there any simple reason that this patch doesn't work? Maybe this is why we'd need those macros?

gmmproc, window, gtk_window_get_size: Example code discarded.
No conversion from Gdk::WindowTypeHint to GdkWindowTypeHint defined (line: 167, parameter name: hint)
m4 failed with exit code 1.  Aborting...
Comment 36 Murray Cumming 2017-04-27 20:53:16 UTC
Created attachment 350597 [details] [review]
0001-GutterRenderer-Move-enums-into-the-class.patch

This is also a bit odd. This patch, for the gtkmm4 branch of gtksourceviewmm, moves some enums into the class, but the default signals handlers, one of which uses an enum, appear before the enum, so the compilation fails.
Comment 37 Kjell Ahlstedt 2017-04-28 06:51:54 UTC
I get the error message:

gmmproc, window, gtk_window_get_size: Example code discarded.
No conversion from Gdk::WindowType::Hint to GdkWindowTypeHint defined (line:
167, parameter name: hint)

Note Gdk::WindowType::Hint instead of your Gdk::WindowTypeHint. That's because
you've written Gdk::WindowType::Hint instead of Gdk::Window::TypeHint.

When that's fixed, you'll notice that you must also change Gtk::Menu::
property_menu_type_hint().
Comment 38 Murray Cumming 2017-04-28 07:35:17 UTC
Comment on attachment 350561 [details] [review]
0001-Gdk-Window-Change-WindowTypeHint-to-Window-TypeHint.patch

Thanks for that sanity check.
Comment 39 Kjell Ahlstedt 2017-04-28 11:19:37 UTC
There's a strange difference between _CLASS_GOBJECT and _CLASS_GTKOBJECT.
_CLASS_GOBJECT puts default signal handlers at the end of the class definition,
_CLASS_GTKOBJECT puts them near the beginning.

A few lines near the end of class_gobject.m4 read
  private:
  _IMPORT(SECTION_CLASS2)
  public:
  _H_VFUNCS_AND_SIGNALS()

The corresponding lines in class_gtkobject.m4 read
  _H_VFUNCS_AND_SIGNALS()
  private:
  _IMPORT(SECTION_CLASS2)

I don't understand why anyone would want to have the default signal handlers
near the beginning. An easy fix would be to change class_gtkobject.m4.
That would break ABI (the vtable would change), but that's allowed now.
An alternative would be to have _WRAP_ENUM add forward declarations of all
in-class enums in the m4 section where the default signal handlers are declared.
Comment 40 Murray Cumming 2017-04-28 11:30:47 UTC
Yes, it would be best to just make class_gobject.m4 and class_gtkobject.m4 as similar as possible. We don't need to worry about breaking the ABI in gtkmm4 yet. Could you do that, please?
Comment 41 Kjell Ahlstedt 2017-04-28 16:22:58 UTC
Fixed _CLASS_GTKOBJECT: https://git.gnome.org/browse/gtkmm/commit/?id=5a15bec9ef203e0e7b07bb7694d8009f20cf8e40
Hope it solves the problem with an in-class enum in gtksourceviewmm.
Comment 42 Murray Cumming 2017-04-28 21:23:34 UTC
Thanks. That fixes it.
Comment 43 Kjell Ahlstedt 2017-05-12 14:49:01 UTC
Created attachment 351729 [details] [review]
patch: gmmproc, _WRAP_ENUM: Add optional CONV_TO_INT parameter

Here's a suggestion for enums that shall be implicitly convertible to int.
The generated code is something like:

  class ResponseType_Enum {
  public:
    enum ResponseType {
      NONE = -1,
      REJECT = -2,
      ACCEPT = -3,
      ...
    };
    ResponseType_Enum() = delete;
  };
  using ResponseType = ResponseType_Enum::ResponseType;

In the gtkmm-list thread where we've discussed this issue, we've concentrated
exclusively on scoped enumerators and conversion to int. When I started to test
some code, I realized that it's also useful to be able to use the enum as an
enum! Simple example:
  f(ResponseType response = ResponseType::NONE);
This is perhaps not necessary with Gtk::ResponseType, but Pango::Weight is used
both as a Pango::Weight and as an int.
Comment 44 Daniel Boles 2017-05-13 09:00:13 UTC
Personally I would not suffix the name of a class with _Enum - seems ripe for confusion.

Can't you just call the class ResponseType and leave the enum anonymous? That seems like what Jonathan indicated most recently on the list thread.
Comment 45 Kjell Ahlstedt 2017-05-14 10:18:17 UTC
I tried to make a mixture of a plain (old-style) enum and an enum class.
It shall be implicitly convertible to int, and its enumerators shall be scoped.
In other respects it shall be possible to use it as if it's an enum, e.g.
  ResponseType r = ResponseType::REJECT;
  int i = r;
but not
  r = REJECT;
  i = REJECT;
and preferably not
  r = i;

That's perhaps possible to achieve if ResponseType is the name of the class,
but it won't be a minimal wrapper class.

I'm open to suggestions for the suffix of the class name. _EnumWrapper would be
more correct, but it's rather long.

I'm also thinking of renaming the _WRAP_ENUM() option to INT_CAST, by analogy
with static_cast, const_cast, etc.
Comment 46 Kjell Ahlstedt 2017-06-01 14:07:44 UTC
Can we decide what to do with enums that we want to be implicitly convertible
to int? So far no one has come up with a really good and simple solution.
Probably there is none. Scott Meyers discusses the issue in "Effective Modern
C++", item 10, "Prefer scoped enums to unscoped enums". His suggestion is an
explicit conversion with a template function that can be used instead of
static_cast.

Some possibilities:
1. Implement my patch in comment 43, possibly after minor modifications.
2. Implement something else. There are many suggestions in the discussion on
   the gtkmm-list.
3. Give up, and accept that enums are not implicitly convertible to int. After
   all there are very few that should be.
Comment 47 Kjell Ahlstedt 2017-06-11 11:40:00 UTC
I have pushed a patch very similar to the one in comment 43. The new _WRAP_ENUM
option is still called CONV_TO_INT. The class suffix is _Wrapper, not _Enum.