Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#17671 closed defect (fixed)

Noting Messages deprecated in Scintilla 3.6.6

Reported by: NewPagodi Owned by: Artur Wieczorek <artwik@…>
Priority: normal Milestone:
Component: wxStyledText Version: dev-latest
Keywords: deprecated Cc:
Blocked By: Blocking:
Patch: yes

Description

The scintilla developers have deprecated 7 messages. Three of these messages (SCI_SETSTYLEBITS, SCI_GETSTYLEBITS, and SCI_GETSTYLEBITSNEEDED) are currently handled by wxSTC. The others have either been removed or were never in wxSTC.

I assume the methods handling these messages should be deprecated in wxWidgets as well. This patch simply modifies gen_iface.py so that the headers for the methods handling the 3 deprecated messages are wrapped in the wxDEPRECATED macro.

I don't know if this can be and #16263 can both be applied, but I can resubmit either if needed.

Attachments (3)

deprecated.patch download (546 bytes) - added by NewPagodi 4 years ago.
stcdepitems.patch download (21.6 KB) - added by NewPagodi 3 years ago.
fixstcsample.patch download (505 bytes) - added by NewPagodi 3 years ago.

Download all attachments as: .zip

Change History (18)

Changed 4 years ago by NewPagodi

comment:1 Changed 4 years ago by NewPagodi

I just realized this patch messes up the documentation generated by gen_iface.py. I have another idea that I'll try to implement later.

comment:2 follow-up: Changed 4 years ago by awi

  • Status changed from new to infoneeded_new
  1. Unfortunately, modified script creates in the interface header entries like this one:
    wxDEPRECATED( void SetStyleBits(int bits) );
    

@deprecated annotation in the comment would be a better solution.

  1. Isn't this report & patch superseded by #17680?

comment:3 in reply to: ↑ 2 ; follow-up: Changed 4 years ago by NewPagodi

  • Status changed from infoneeded_new to new

Replying to awi:

  1. Unfortunately, modified script creates in the interface header entries like this one:
    wxDEPRECATED( void SetStyleBits(int bits) );
    

@deprecated annotation in the comment would be a better solution.

This was how it this patch messes up the generated documentation. Sorry about that.


  1. Isn't this report & patch superseded by #17680?

The other idea I mentioned above is that after #17680 is applied, we could add a new list to gen_iface.py that would be something like:

deprecatedItems = [ ... ]

and then modify the processIface function to use that list so that the deprecated items get wrapped with the wxDEPRECATED macro in the generated source code header but not in the generated interface header. I could also have it automatically add the @deprecated annotation in the generated interface header.

If you don't think that's a good idea, then this ticket should be closed.

comment:4 in reply to: ↑ 3 Changed 4 years ago by awi

Replying to NewPagodi:

deprecatedItems = [ ... ]

and then modify the processIface function to use that list so that the deprecated items get wrapped with the wxDEPRECATED macro in the generated source code header but not in the generated interface header. I could also have it automatically add the @deprecated annotation in the generated interface header.

If you don't think that's a good idea, then this ticket should be closed.

I think the future-proof solution would be to extract deprecated functions from the Deprecated category in Scintilla.iface. AFAICS, gen_iface.py just ignores deprecated functions now.
Unfortunately, Scintilla.iface contents not always reflects actual status of the library, e.g. SCI_SETSTYLEBITS, SCI_GETSTYLEBITS, and SCI_GETSTYLEBITSNEEDED are claimed as deprecated since 3.4.3 (per documentation) or 3.4.4 (per release notes), but in 3.7.2 corresponding APIs are still not moved to the Deprecated category (see Scintilla bug #1908). So, some fallback mechanism to handle such discrepancies would be welcome.

unlistedDeprecatedItems = [ ... ]

or something?

comment:5 Changed 3 years ago by NewPagodi

Thanks to awi for working with the previous set of wxSTC patches. With #17680 out of the way, I think I can just add a few lines to gen_iface.py to add deprecated messages only in the source code header for these items.

While I'm working on that, here are some questions I have related to these particular functions and deprecated scintilla items in general:

1) as mentioned above, the current behavior of gen_iface.py is to stop reading when it gets to the deprecated items. But the link for the scintilla bug says that these items will be moved to that category soon. So I think we should change that behavior so that items in the deprecated section are included but noted as deprecated. Are there any objections?

2) Also in the iface file, there is a section of provisional items. That section is currently empty, but I think we should also change the behavior of gen_iface.py to exclude these items if there ever are any. Are there any objections?

3) Related to (2), although the provisional items section is currently empty, there are 2 scintilla values that should be listed in it. They are inputs for the SetTechnology which as mentioned in #17804 aren't implimented, so I guess they're not doing any harm at this time. But it will still probably be best if they were excluded from wxSTC.

So I guess there should be some way to exclude some scintilla values from having a generated corresponding wxSTC value. I can't think of a good way to do that. Are there any suggestions?

4) Somewhat related to (1) and (3), the values INDIC0_MASK, INDIC1_MASK, INDIC2_MASK, and INDICS_MASK are only used with the the 3 functions discussed above, so it might be a good idea to deprecate those values as well. I think I might have a way to do that. Instead of letting those values be defined with macros, instead declare an enum like:

enum wxSTC_Indic_Masks
{
    wxSTC_INDIC0_MASK=0x20,
    wxSTC_INDIC1_MASK=0x40,
    wxSTC_INDIC2_MASK=0x80,
    wxSTC_INDICS_MASK=0xE0
}

The problem is that the syntax for declaring an enum deprecated varies from compiler to compiler. So I guess we would need something like a macro wxDEPRECATED_ENUM(enumname,msg) that would be analogous to the existing wxDEPRECATED_MSG(msg) macro. This might also provide a better solution for #17808 .

If this seems like a good idea, I would need help writing that macro. If that doesn't seem like a good idea, then never mind.

5) When I upgraded Scintilla to 3.7.2 in #17705, the signature of the StartStyling function had changed from 'void StartStyling(int pos, int mask);' to 'void StartStyling(int start);'. This was handled, by adding an entry in the method override map to give a default value for the second parameter.
But I thought a better way to handle this might be to let the new version be autogenerated but manually declare the old version as an overload version with a deprecation message in stc.h.in and stc.cpp.in similarly to how UsePopUp was implimented.
I think that would be a better way, but it is a pretty small issue. Is that worth doing?

comment:6 Changed 3 years ago by awi

Comments from my side:
1) No objections.
2) No objections, but it would be nice to generate a note about provisional status in the documentation too.
3) Maybe these Scintilla values to be excluded from mapping to wxSTC values should be held in the yet another list/tuple in gen_iface.py, say 'notMappedValues', and all values present in this tuple would be filtered out during processing?
4) I think that items (functions and values) which are not present in the 'deprecated' category in scintilla.iface but which are actually deprecated (e.g. noted as deprecated in Scintilla doc) should be also listed in the dedicated list/tuple in gen_iface.py and marked as deprecated (in the header and doc) during processing.
The concept of encapsulating deprecated constants in the enum and declaring it as deprecated is fine. But bear in mind that wx developers use really various compilers, in various modes, so please be careful with this implementation of portable solution.
5) It's better (more clear and transparent) to explicitly declare old version as deprecated instead of doing tricks with parameters.

comment:7 follow-up: Changed 3 years ago by NewPagodi

I've prepared a patch. It doesn't change very many lines, but it does a lot of things:

1) adds a global variable GENERATE_PROVISIONAL_ITEMS to gen_iface.py similar to the existing FUNC_FOR_CMD global variable. When it is set to 1, methods and values in the provisional section of Scintilla.iface receive cooresponding STC items, but a line like "This method is provisional and is subject to change in future versions of wxStyledTextCtrl." is added to their documentation. When set to 0, those items are suppressed. I've tested this by manually cutting items from elsewhere in the iface file and pasting them in that section. I currently have the value set at 0.

2) adds a set notMappedSciValues that can be used to prevent the creation of specific STC values. I've added the 6 values to it - 2 because they aren't actually implimented in wxSTC, and 4 'INDIC?_MASK' values that I want to handle manually.

3) I've changed the behavior of the processIface function so that it doesn't stop when it reaches the deprecated section of the file. Instead it reads till the end of the file but tracks which section a function or value is in.

4) I've altered the parseVal, parseFun, buildDocs, and processMethods functions to use that extra information - provisional items get the notes mentioned above. Functions and values in the deprecated section have an @deprecated annotation added to their docs. Methods will also have this line:

wxDEPRECATED_MSG( "This method uses a function deprecated in the Scintilla library." )

inserted between the docstring and the declaration in the source code header file but not in the interface header file. I can change that message if another one is preferred.

Methods listed in the "Deprecated items" section of the documentation get the same treatment.

5) adds a wxSTC_DEPRECATED_ENUM(name,msg) as described above. It is based on the wxDEPRECATED_MSG macro. I've tested it with Clang in linux, GCC (in linux and with TDM-GCC in windows), and with visual c++ 2010 and 2015. Visual c 2010 doesn't print the warning, but it does accept the syntax the macro uses.

6) uses that macro to declare the wxSTC_INDIC{0,1,2,S}_MASK values deprecated. I've also manually added them to stc.interface.h.in and added @deprecated annotations for them.

7) uses that macro to redo the delcrations for wxSTC_SCMOD_ and wxSTC_SCVS_ values so that a warning is supplied when someone tries to use those old values.

8) Adds a second overload of the StartStyling function as described above.

The net result of all of this:

  • I think all think all of the functions and values related to style byte indicators that were removed in Scintilla 3.4.4 now have some kind of warning telling users not to use them.
  • I think this adds useful system for handling the items in the deprecated and provisional sections of Scintilla.iface

comment:8 follow-up: Changed 3 years ago by NewPagodi

Also to awi, when this ticket is closed would you be willing to look at #17305 and/or #16263. Those are the last 2 issues I want to provide patches for at this time. They're both much simpler than any this last set STC patches that have been applied over the past few weeks, but I'll understand if you're tired of dealing with wxSTC issues for now.

comment:9 in reply to: ↑ 8 Changed 3 years ago by awi

Replying to NewPagodi:

Also to awi, when this ticket is closed would you be willing to look at #17305 and/or #16263.

No problem, we can take a look at them.

comment:10 in reply to: ↑ 7 ; follow-up: Changed 3 years ago by awi

All modifications look generally OK, but I have some remarks regarding points 5, 6, 7, those about introducing macro wxSTC_DEPRECATED_ENUM to mark enums deprecated.
I tested this macro with GCC 4.9.2 and VS2015 and warning was only generated if enum name was explicitly referred in the statement, e.g.:

// For VS2015
int n = wxSTC_Indic_Masks::wxSTC_INDIC0_MASK;
// For GCC
wxSTC_Indic_Masks n = wxSTC_INDIC0_MASK;

If it was used in a standard (more typical, I think) way, without referring enum, like this way:

int n = wxSTC_INDIC0_MASK;

no warning was generated.

For VS2012 no warning was generated even if the fully qualified form was used.

Could you please double check this on your side?

NB: Under VS #pragma deprecated(name) seems to work quite fine, even for individual enum items.

I think that only technique which doesn't require explicit reference to the enum name could be usable.

comment:11 in reply to: ↑ 10 Changed 3 years ago by NewPagodi

Replying to awi:

All modifications look generally OK, but I have some remarks regarding points 5, 6, 7, those about introducing macro wxSTC_DEPRECATED_ENUM to mark enums deprecated.
I tested this macro with GCC 4.9.2 and VS2015 and warning was only generated if enum name was explicitly referred in the statement, e.g.:

You're right. What I had only worked with clang. In testing, I just declared variables of the deprecated type and saw they were getting warnings and stopped there. That's, of course, not how an STC user would use them. When the enum entries are used as an actual wxSTC user would do, no warnings are produced. I don't know what I was thinking.

I've updated the patch to try a second approach. This uses the technique discussed here: http://stackoverflow.com/questions/2681259/how-to-deprecate-a-macro-in-gcc to deprecate the macro definitions with clang and gcc.

I've tested this with all 3 compilers and ensured that actually work as they're supposed to. There's still 2 potential problems though:

1) this requires the _Pragma operator. According to that link, it's been available by default in GCC since 4.5. (And was available long before that if the language standard was set to c99 ). However, I can't seem to find if it was available in clang from the start or added later.

#if defined(__clang__) || wxCHECK_GCC_VERSION(4, 5)

An extra check may be to limit the clang versions the deprecation macro is used with. But since that is a c99 operator and the first version of clang came out years after c99, it's probably safe to assume it had it from the start. Does anyone know for certain?

2) visual c uses a completely different system for deprecating macros, so each set of deprecated definitions begins with a set of stuff only for visual c. The result is pretty ugly. Also, I don't think you can add a message, so all you get are warnings like "warning C4995: 'wxSTC_INDICS_MASK': name was marked as #pragma deprecated" that may not be very helpful.

Changed 3 years ago by NewPagodi

comment:12 Changed 3 years ago by Artur Wieczorek <artwik@…>

  • Owner set to Artur Wieczorek <artwik@…>
  • Resolution set to fixed
  • Status changed from new to closed

In 15ee1ebd12ee6e6807ec3464fd0954c67f6dbf4e/git-wxWidgets:

Improve generating wxSTC code and documentation

Several new features were implemented in gen_iface.py and accompanying '.in' files:

  1. Added an option to parse provisional section in Scintilla.iface. When global variable GENERATE_PROVISIONAL_ITEMS is set to 1, methods and values in this section receive corresponding wxSTC items.
  2. Deprecated section in Scintilla.iface is not skipped, but parsed and corresponding wxSTC items are created (marked as deprecated in the code and documentation).
  3. Added a set 'notMappedSciValues' that can be used to prevent the creation of specific wxSTC values which are intended to be handled manually.
  4. For deprecated constants there is generated a code to raise respective warnings during the compilation.

Closes #17671.

comment:13 follow-up: Changed 3 years ago by awi

It would be godd to remove from stc sample a call to the deprecated SetStyleBits() function.

Changed 3 years ago by NewPagodi

comment:14 in reply to: ↑ 13 Changed 3 years ago by NewPagodi

Replying to awi:

It would be godd to remove from stc sample a call to the deprecated SetStyleBits() function.

I've attached a small patch to do this.

comment:15 Changed 3 years ago by Artur Wieczorek <artwik@…>

In f33da324c06545a99d3d5af62876fc3dab5848fd/git-wxWidgets:

Don't use deprecated function in stc sample

Call to deprecated wxStyledTextCtrl::SetStyleBits() function can be omitted because underlying SCI_SETSTYLEBITS API is not operational anymore.

See #17671.

Note: See TracTickets for help on using tickets.