#14784 closed enhancement (fixed)

protect definitions in log.h interface file respectively to their wxlib

Reported by: phi Owned by:
Priority: normal Milestone: 2.9.5
Component: documentation Version: stable-latest
Keywords: Cc:
Blocked By: Blocking:
Patch: yes

Description

To be able to run SWIG for each wxlib, it is easier to have them in separate files.
BTW, I reorganized the order in which the classes were defined, parent classes first. (doxygen doesn't care, SWIG does).

Attachments (2)

fix_log_doc.patch download (34.7 KB) - added by phi 21 months ago.
fix_log_doc_preprocessor.patch download (71.2 KB) - added by phi 21 months ago.

Download all attachments as: .zip

Change History (14)

comment:1 follow-up: Changed 21 months ago by vadz

Why does the patch move things around in log.h itself, is there any need to do this? It would be preferable to not do it if it's not necessary.

Also, as discussed on wx-dev, could you please add @header{wx/log.h} to everything in wx/generic/logg.h? TIA!

comment:2 Changed 21 months ago by vadz

  • Status changed from new to infoneeded_new

comment:3 in reply to: ↑ 1 Changed 21 months ago by phi

Replying to vadz:

Why does the patch move things around in log.h itself, is there any need to do this? It would be preferable to not do it if it's not necessary.

Maybe I was not so clear in the description.
The classes in log.h are put randomly in the file, with the issue that some parent classes (rake wxLog) are defined after subclasses. This is not a problem for doxygen but is for any compiler which SWIG is a sort of.

Also, as discussed on wx-dev, could you please add @header{wx/log.h} to everything in wx/generic/logg.h? TIA!

Done.

BTW, I also used -d to generate the patch, so it is more human readable.

Regards

Changed 21 months ago by phi

comment:4 follow-up: Changed 21 months ago by vadz

OK, thanks for the explanation, I indeed misunderstood/missed it the first time.

I'll apply this now but just one question: what about the virtual dtors addition? Is this really needed? If so, why, and should we also check add them to all the other classes?

comment:5 Changed 21 months ago by VZ

  • Resolution set to fixed
  • Status changed from new to closed

(In [72818]) Split documentation of the GUI wxLog classes in a separate file.

Put the GUI classes in a new interface/wx/generic/logg.h to make it easier to
generate wrappers for only the base or only the core libraries.

Also change the order of the classes remaining in log.h to ensure that the
base classes are always defined before the derived ones.

Closes #14784.

comment:6 in reply to: ↑ 4 ; follow-up: Changed 21 months ago by phi

Replying to vadz:

I'll apply this now but just one question: what about the virtual dtors addition? Is this really needed? If so, why, and should we also check add them to all the other classes?

Oh yes, I forgot I already included this change. I wished to do it in a separate ticket to address them all.

So, the story is: Yes we should add virtual to the destructors that are in the real headers, SWIG outputs a warning when a method is virtual but the destructor is not (like the compilers). I'm not quite sure but I think the generated wrapper may leak memory if the destructor is not virtual.

BTW, was it a "feature" to not declare them as virtual for the docs?

Regards

comment:7 in reply to: ↑ 6 Changed 21 months ago by vadz

Replying to phi:

Oh yes, I forgot I already included this change. I wished to do it in a separate ticket to address them all.

This would be indeed better, TIA.

So, the story is: Yes we should add virtual to the destructors that are in the real headers, SWIG outputs a warning when a method is virtual but the destructor is not (like the compilers).

If this is the only problem, I think it'd be better to just disable the warning (which can be done easily with SWIG).

I'm not quite sure but I think the generated wrapper may leak memory if the destructor is not virtual.

Hmm, this would be somewhat surprising but if it's a real problem, then we should indeed add virtual dtors declarations.

BTW, was it a "feature" to not declare them as virtual for the docs?

I think we do document them as being virtual for the classes that are supposed to be used polymorphically and deleted from user code. But just having virtual dtors in the documentation without even saying anything about them just doesn't seem very useful...

comment:8 Changed 21 months ago by vadz

  • Milestone set to 2.9.5
  • Resolution fixed deleted
  • Status changed from closed to reopened

As discussed in #14785, there is a problem with this one too: wxLogGui documentation now mentions wx/generic/logg.h although it should be wx/log.h. I don't know why doesn't @header work here but I don't have time (nor probably the expertise) to find out so I think this one should be reverted too and the #if wxUSE_GUI checks added.

comment:9 Changed 21 months ago by VZ

(In [72832]) Revert "Split documentation of the GUI wxLog classes in a separate file."

This reverts r72818 as it resulted in wrong header files being generated in
the documentation for the GUI wxLog classes.

See #14784.

comment:10 Changed 21 months ago by vadz

  • Milestone 2.9.5 deleted
  • Patch unset
  • Priority changed from normal to low

comment:11 Changed 21 months ago by phi

  • Milestone set to 2.9.5
  • Patch set
  • Priority changed from low to normal
  • Summary changed from Split log.h interface file so that each file addresses a unique wxlib to protect definitions in log.h interface file respectively to their wxlib

Maybe not too late for 2.9.5

Here is a new patch that introduces preprocessor condtionals to protect definitions using wxUSE_BASE and wxUSE_GUI.

Note that the patch looks ugly because I had to move classes around to put parent classes first as described in earlier comments.

Note that it should be commited in the same changeset than #14785, so that doxygen will use the preprocessor definitions.

Changed 21 months ago by phi

comment:12 Changed 21 months ago by VZ

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [72911]) Add wxUSE_BASE checks for wxLog classes in the interface header.

Also rearrange them so that the derived classes always appear after the base
ones, otherwise SWIG can't compile this file.

Closes #14784.

Note: See TracTickets for help on using tickets.