These are the files from the swig/classes/include directory. I hope the line endings don''t end up wrong. _______________________________________________ wxruby-users mailing list wxruby-users@rubyforge.org http://rubyforge.org/mailman/listinfo/wxruby-users
On Tue, 2006-08-29 at 22:54 -0400, Roy Sutton wrote:> These are the files from the swig/classes/include directory. I hope the > line endings don''t end up wrong.I''m replying to both sets of patches at once. There are enough areas of concern that I can''t take the patches as-is. I can either apply parts of it (on a file-by-file basis), or have you resubmit separate patches for the different categories. Your choice. Most of my concerns are related to methods that appear to really be virtual, but which you have un-virtual''d. It''s not clear to me why you are making that change, nor whether the change should be temporary or permanent. Perhaps really clear comments would help me to understand. I would be happy with a patch for the Event classes, removing Clone. I would also be happy with a patch for the ControlWithItems classes, especially with a clearer comment about which methods should be removed (like have a comment ending the to-be-removed section). Frame doesn''t look right, though. You took away the virtual from Create/OnCreateStatus/ToolBar, and clearly those methods are virtual. If the OnXxx methods aren''t virtual, there is no point in exposing them. I assume you un-virtual''d them because they are actually defined in the BaseFrame class which we are not yet wrapping. I would like to see a comment to that effect, rather than just taking out the virtual keyword without explanation. In Grid, all the methods that you de-virtualized appear to be virtual to me. Same with GridCellBoolRenderer. I didn''t check the rest of the grid classes, guessing they have the same problem. The un-virtual''d methods in Sizer and TextCtrl don''t look right. You removed several methods from TextValidator for no apparent reason. All the changes in Window looked good, aside from some //TODO was virtual comments that are not near any changes. In App.i, let''s remove CreateLogTarget since it is deprecated. And GetTopWindow looks virtual to me. Kevin
Kevin Smith wrote:> I''m replying to both sets of patches at once. > > There are enough areas of concern that I can''t take the patches as-is. I > can either apply parts of it (on a file-by-file basis), or have you > resubmit separate patches for the different categories. Your choice. >I''ll try to break them up better by functional area> Most of my concerns are related to methods that appear to really be > virtual, but which you have un-virtual''d. It''s not clear to me why you > are making that change, nor whether the change should be temporary or > permanent. Perhaps really clear comments would help me to understand. >I thought I had explained this in an e-mail but I guess I didn''t. The virtual functions which return a pointer generate a SWIG warning and those functions, when used, often cause wxRuby to crash. In many cases, the functions really wouldn''t be overridden by Ruby-derived classes. My intention was to remove them until we could decide if there was a safe way to expose them. The changes -should- only be temporary if a solution can be found.> I would be happy with a patch for the Event classes, removing Clone. I > would also be happy with a patch for the ControlWithItems classes, > especially with a clearer comment about which methods should be removed > (like have a comment ending the to-be-removed section). >I can resubmit the Event classes as separate files. ControlWithItems patches: if I have to document them that well I might as well just make the change. It could be a couple days.> Frame doesn''t look right, though. You took away the virtual from > Create/OnCreateStatus/ToolBar, and clearly those methods are virtual. If > the OnXxx methods aren''t virtual, there is no point in exposing them. >Mostly I removed them for the reason above. However, the comment reflects the fact that most of those OnXxxx functions in wxWidgets are protected, internal functions. I see now from the docs they''re not so the ''should we expose this'' comment should go away at least.> I assume you un-virtual''d them because they are actually defined in the > BaseFrame class which we are not yet wrapping. I would like to see a > comment to that effect, rather than just taking out the virtual keyword > without explanation. > > In Grid, all the methods that you de-virtualized appear to be virtual to > me. Same with GridCellBoolRenderer. I didn''t check the rest of the grid > classes, guessing they have the same problem. The un-virtual''d methods > in Sizer and TextCtrl don''t look right. You removed several methods from > TextValidator for no apparent reason. >re TextValidator and other validator classes: Those methods were defined on the parent classes.> All the changes in Window looked good, aside from some //TODO was > virtual comments that are not near any changes. >Ah, well, those methods -should- be virtual, but aren''t. Obviously we had crash bugs associated with those functions when they were virtual. Ideally we''d make them virtual if we figure out how to fix the problem with directors.> In App.i, let''s remove CreateLogTarget since it is deprecated. And > GetTopWindow looks virtual to me. >I''m OK with that.> Kevin >I''ll see about grouping and resubmitting patches. As I said, it was a large number of patches. They''re all themed around making the header files match the structure of wxWidgets better or addressing the shortcomings of SWIG wrapping virtual functions that return pointers. Roy
Roy Sutton wrote:> I thought I had explained this in an e-mail but I guess I didn''t. The > virtual functions which return a pointer generate a SWIG warning and > those functions, when used, often cause wxRuby to crash. In many cases, > the functions really wouldn''t be overridden by Ruby-derived classes. My > intention was to remove them until we could decide if there was a safe > way to expose them. The changes -should- only be temporary if a > solution can be found.The problem is that if a function really is virtual in wx, but we claim that it is not, we can also get into problems. Possibly even crashes, although I''m not certain. So I''m not sure it''s worth temporarily making things less correct unless there is a very clear net benefit.> I can resubmit the Event classes as separate files. ControlWithItems > patches: if I have to document them that well I might as well just make > the change. It could be a couple days.No need for an extensive comment. I''ll take the ControlWithItems as is if necessary, although a single line like // End ControlWithItems at the bottom of the group of methods would be great. Not required.>> Frame doesn''t look right, though. You took away the virtual from >> Create/OnCreateStatus/ToolBar, and clearly those methods are virtual. If >> the OnXxx methods aren''t virtual, there is no point in exposing them. >> > Mostly I removed them for the reason above. However, the comment > reflects the fact that most of those OnXxxx functions in wxWidgets are > protected, internal functions. I see now from the docs they''re not so > the ''should we expose this'' comment should go away at least.I can''t imagine any OnXxx method being protected or internal.> re TextValidator and other validator classes: Those methods were > defined on the parent classes.Still, if they are virtual in a wx class, I think we need to declare them so in our headers. Otherwise we get into weird troubles.>> All the changes in Window looked good, aside from some //TODO was >> virtual comments that are not near any changes. >> > Ah, well, those methods -should- be virtual, but aren''t. Obviously we > had crash bugs associated with those functions when they were virtual. > Ideally we''d make them virtual if we figure out how to fix the problem > with directors.I guess I would rather %ignore them than declare them incorrectly.> I''ll see about grouping and resubmitting patches. As I said, it was a > large number of patches. They''re all themed around making the header > files match the structure of wxWidgets better or addressing the > shortcomings of SWIG wrapping virtual functions that return pointers.Thanks! I hope this email clarifies what I would like. To summarize: Event patch great, ControlWithItems patch fine (bonus points for slightly better comments). Declaring things non-virtual that are virtual in wx seems bad. Using %ignore to work around virtual problems would be fine. Kevin
Kevin Smith wrote:> The problem is that if a function really is virtual in wx, but we claim > that it is not, we can also get into problems. Possibly even crashes, > although I''m not certain. >The only difference to us declaring it virtual or not is that SWIG will either generate a director wrapper or not. Even using %ignore will (with the current released SWIG) cause problems since it incorrectly wraps %ignored functions. You can still call the function directly from Ruby, you just can''t override the function in a derived class. I maintain that in almost every case we won''t have any issue whatsoever. Does it rub me the wrong way that these functions are misdeclared? Yes, it does.> Still, if they are virtual in a wx class, I think we need to declare > them so in our headers. Otherwise we get into weird troubles. >I don''t /believe/ this is the case, as has been proven by the fact we''ve had the virtual/non virtual headers wrong for a long time. I /suppose/ it could cause an issue for calling from Ruby. It won''t hurt wx''s internal calls at all since they are already compiled correctly. AFAIK there is no need to declare a method virtual more than once in its parent''s hierarchy. SWIG will generate a wrapper at each level for any virtual function. It only has to be declared virtual once. If anyone knows better than that please let me know. Roy
Roy Sutton wrote:> Kevin Smith wrote: >> The problem is that if a function really is virtual in wx, but we claim >> that it is not, we can also get into problems. Possibly even crashes, >> although I''m not certain.Ok. I thought this through, and I think I agree with you. The dangerous case I was thinking of is more complicated and less common.> Does it rub me the wrong way that these functions are misdeclared? Yes, > it does.Here''s a proposal, then: Instead of removing the virtual keyword, let''s replace it with a SHOULD_BE_VIRTUAL placeholder which can be #define''d to nothing. As we fix each one, we can restore the correct virtual keyword.> AFAIK > there is no need to declare a method virtual more than once in its > parent''s hierarchy. SWIG will generate a wrapper at each level for any > virtual function. It only has to be declared virtual once.The problems I encountered mostly had to do with "missing" layers, where we were wrapping the top-level class and the bottom-level, but not the one in the middle. Now that I understand the issues, I can go back through your patches and deal with them, if you want. It would save me some time to get new versions from you, but if you are short of time I can do it this weekend. Thanks, Kevin