Neil Hodgson | 10 Jul 2012 10:20

.po file lexer uses static variable

   The lexer for .po translation files, which is located inside LexOthers.cxx as ColourisePoDoc /
ColourisePoLine, uses a static variable 'state'. This appears to be unsafe and is likely to be incorrect
when called multiple times or when multiple files are loaded. If anyone is using this lexer, it should be
modified to not use any static variables: it is likely what it wants is to continue on from the pressing
style which is the 'initStyle' (3rd) parameter to the lexer.

   Neil

--

-- 
You received this message because you are subscribed to the Google Groups "scintilla-interest" group.
To post to this group, send email to scintilla-interest <at> googlegroups.com.
To unsubscribe from this group, send email to scintilla-interest+unsubscribe <at> googlegroups.com.
For more options, visit this group at http://groups.google.com/group/scintilla-interest?hl=en.

Colomban Wendling | 8 Sep 2012 22:16
Gravatar

Re: .po file lexer uses static variable

If anybody received my previous empty message, I'm sorry.  I messed up
with my mail client ending up sending the message before writing it...
Fortunately, it seems to have been stopped at some point, so maybe
nobody got it.

Le 10/07/2012 10:20, Neil Hodgson a écrit :
> The lexer for .po translation files, which is located inside
> LexOthers.cxx as ColourisePoDoc / ColourisePoLine, uses a static
> variable 'state'. This appears to be unsafe and is likely to be
> incorrect when called multiple times or when multiple files are
> loaded. If anyone is using this lexer, it should be modified to not
> use any static variables: it is likely what it wants is to continue
> on from the pressing style which is the 'initStyle' (3rd) parameter
> to the lexer.

Trying to fix this I ended up writing a new and a little more complete
lexer.  It behaves like the old one but follows a little better the
language like defined by Gettext[1] and as understood by Gettext's
`msgftm` tool; and provides a few more styles [2].

However, I'm not 100% sure about how I dealt with what used the static
variable before.  The thing is that there are 3 string styles, and the
chosen one depends on what precedes it, which can be lines before, and
moreover that is separated by the default style.  For example, given the
code below:

	1. msgid "foo"
	2. "foo string continues"
	3. msgstr "bar"
	4.
(Continue reading)

Neil Hodgson | 9 Sep 2012 00:33

Re: .po file lexer uses static variable

Colomban Wendling:

> If anybody received my previous empty message, I'm sorry.

   Your account still had the moderate flag on, so I saw it and removed it.

> However, I'm not 100% sure about how I dealt with what used the static
> variable before.  The thing is that there are 3 string styles, and the
> chosen one depends on what precedes it, which can be lines before, and
> moreover that is separated by the default style.

   The common ways of implementing this without introducing user-visible changes are:

1) Use line states to remember the state at the end of each line. styler.SetLineState / styler.GetLineState

2) Seek back to the start of the feature and discover the state there.

3) Write an object lexer and record each change in the feature state. This is much more work but is better for
complex functionality.

   Neil

--

-- 
You received this message because you are subscribed to the Google Groups "scintilla-interest" group.
To post to this group, send email to scintilla-interest <at> googlegroups.com.
To unsubscribe from this group, send email to scintilla-interest+unsubscribe <at> googlegroups.com.
For more options, visit this group at http://groups.google.com/group/scintilla-interest?hl=en.

Colomban Wendling | 9 Sep 2012 02:33
Gravatar

Re: .po file lexer uses static variable

Le 09/09/2012 00:33, Neil Hodgson a écrit :
> Colomban Wendling:
> 
>> If anybody received my previous empty message, I'm sorry.
> 
> Your account still had the moderate flag on, so I saw it and removed
> it.

Oh, so I haven't ever posted on the ML before?  Well, maybe it was only
on the trackers.  Anyway that's fortunate, and thanks :)

>> However, I'm not 100% sure about how I dealt with what used the
>> static variable before.  The thing is that there are 3 string
>> styles, and the chosen one depends on what precedes it, which can
>> be lines before, and moreover that is separated by the default
>> style.
> 
> The common ways of implementing this without introducing user-visible
> changes are:
> 
> 1) Use line states to remember the state at the end of each line.
> styler.SetLineState / styler.GetLineState

That's perfect, thanks!  I updated my patch (attached) to use this
instead of the extra styles and it works just fine, removing the weird
styles and even some lines of code.

> 2) Seek back to the start of the feature and discover the state
> there.
>
(Continue reading)

Neil Hodgson | 10 Sep 2012 05:12

Re: .po file lexer uses static variable

Colomban Wendling:

> That's perfect, thanks!  I updated my patch (attached) to use this
> instead of the extra styles and it works just fine, removing the weird
> styles and even some lines of code.

   cppcheck complains about duplicate if/else branches for lines 693/695. While it looks like you plan to
differentiate here, its not unusual for such plans to not be completed leaving confusing code.

   Including .po support in LexOthers was expedient but it should have its own file so it can be included and
excluded easily.

   Neil

--

-- 
You received this message because you are subscribed to the Google Groups "scintilla-interest" group.
To post to this group, send email to scintilla-interest <at> googlegroups.com.
To unsubscribe from this group, send email to scintilla-interest+unsubscribe <at> googlegroups.com.
For more options, visit this group at http://groups.google.com/group/scintilla-interest?hl=en.

Colomban Wendling | 11 Sep 2012 19:49
Gravatar

Re: .po file lexer uses static variable

Le 10/09/2012 05:12, Neil Hodgson a écrit :
> Colomban Wendling:
> 
>> That's perfect, thanks!  I updated my patch (attached) to use this 
>> instead of the extra styles and it works just fine, removing the
>> weird styles and even some lines of code.
> 
> cppcheck complains about duplicate if/else branches for lines
> 693/695. While it looks like you plan to differentiate here, its not
> unusual for such plans to not be completed leaving confusing code.

OK, I'll merge the branches.

> Including .po support in LexOthers was expedient but it should have
> its own file so it can be included and excluded easily.

OK, I'll try to move it to its own file and integrate that.

Regards,
Colomban

--

-- 
You received this message because you are subscribed to the Google Groups "scintilla-interest" group.
To post to this group, send email to scintilla-interest <at> googlegroups.com.
To unsubscribe from this group, send email to scintilla-interest+unsubscribe <at> googlegroups.com.
For more options, visit this group at http://groups.google.com/group/scintilla-interest?hl=en.

Neil Hodgson | 12 Sep 2012 08:03

Re: .po file lexer uses static variable

Colomban Wendling:

> OK, I'll try to move it to its own file and integrate that.

   I accidentally committed your current patch. I'd still like the updates though.

   Neil

--

-- 
You received this message because you are subscribed to the Google Groups "scintilla-interest" group.
To post to this group, send email to scintilla-interest <at> googlegroups.com.
To unsubscribe from this group, send email to scintilla-interest+unsubscribe <at> googlegroups.com.
For more options, visit this group at http://groups.google.com/group/scintilla-interest?hl=en.

Colomban Wendling | 13 Sep 2012 21:17
Gravatar

Re: .po file lexer uses static variable

Le 12/09/2012 08:03, Neil Hodgson a écrit :
> Colomban Wendling:
> 
>> OK, I'll try to move it to its own file and integrate that.
> 
> I accidentally committed your current patch. I'd still like the
> updates though.

Here they are.

0001-Move-PO-lexer-outside-LexOthers.patch:
	Moves the PO lexer to it own file.  Tested build with gtk, and
	grepped for others, hopefully it's OK everywhere (I ran
	LexGen.py).  I also changed the lexer module name from "lmPo"
	to "lmPO" (uppercase) to match other lexer names (like lmCPP,
	lmVB, etc.) since "po" here is an abbreviation.
	I'm not completely sure of the implications though, if it
	brings compatibility issues I can change that back to the old
	"lmPo" name.

0002-Remove-duplicated-branch-in-PO-lexer.patch:
	Removes the duplicated branch.

Regards,
Colomban

--

-- 
You received this message because you are subscribed to the Google Groups "scintilla-interest" group.
To post to this group, send email to scintilla-interest <at> googlegroups.com.
To unsubscribe from this group, send email to scintilla-interest+unsubscribe <at> googlegroups.com.
(Continue reading)

Neil Hodgson | 14 Sep 2012 03:10

Re: .po file lexer uses static variable

Colomban Wendling:

> Here they are.

   OK, committed.

   Neil

--

-- 
You received this message because you are subscribed to the Google Groups "scintilla-interest" group.
To post to this group, send email to scintilla-interest <at> googlegroups.com.
To unsubscribe from this group, send email to scintilla-interest+unsubscribe <at> googlegroups.com.
For more options, visit this group at http://groups.google.com/group/scintilla-interest?hl=en.


Gmane