error in wxSmith - wxsitem.cpp

[frithjofh]

Hi everybody,

I found a bug in wxSmith. It affects the code generated by wxsitem.cpp line 793. There a double value is streamed into a wxString using the << operator. In some cases this leads to wrong results, I think depending on the localization of the OS.

For instance in the case of a wxSplitterwindow one can give a value for the sash gravity in the properties grid. The value has to be given as decimal value with a comma as decimal separator, point is not accepted. The value is correctly transmitted to the according wxs file, using a comma as decimal separator too. This value gets treated in wxssplitterwindow.cpp , where it is converted from wxString to double, from there a call to a function codef in wxsitem.cpp is done. In the above mentioned line 793 in wxsitem.cpp the double value is attached to the wxString representing the actual code in the source file produced by wxSmith. There the operator << is used for the matter. The problem is, the correct double value is changed to a string with a comma as separator and appears like that in the generated code instead of a string with a point decimal separator as it should be.
Compiling the generated code gives an error complaining about no known function SetSashGravity with two ints as arguments.

I'm encountering this behavior on a SuSE 12.1 x64 localized in German language using the last svn version 7930.

Regards

frithjofh

[frithjofh]

on my locale copy of wxSmith source I introduced a change hopefully resolving the problem: in the actual function holding the logic of code generation y put a new line at the beginning: setlocale(LC_NUMERIC, "C"); and another new line at the end: setlocale(LC_NUMERIC, "");

this sets the locale during all of the code generation and then restores it to what it was before. might not be the right way, nor the right place, but as setlocale() is ANSI standard I think it is portable.

on my machine and os it resolves my problem, but I have too little experience to know if this is the right way. I would think there was a more central point in wxSmith up the flow of code generation were a change should be made.

opinions please...

regards

frithjofh

[oBFusCATed]

Using setlocale(...) is a hack, which should not be used, because it will break code running in other threads or code running in a wx event.

[frithjofh]

Oh well,... there's always something.

Haven't thought about neither, really. But the problem is a real one I think. All functions in code::blocks which do textual code generation face it.

So would it be the way to change just the function, to not rely on operator<< , not mingle around with locales, but to parse the given strings themselves and watch for correct decimal separator in the output?

regards

frithjofh

[oBFusCATed]

I'm not really sure what is the correct approach here.

[frithjofh]

Hi again,

after giving it another thought, I only changed the function locally that generates the float representation as string.

I used the <clocale> localeconv() function to find the current decimal separator symbol and do a replace. Not elegant, but it seems to work. I think it does not interfere with threads or events.

See the attached patch file...

regards

frithjof

PS: I think however that there are problems in wxSmith like not checking if input are valid at all, like numbers, that are out of range, and such things. It makes heavy use of rather complicated macros too ...

[oBFusCATed]

localeconv() is not thread safe, too. Also what happens if the functions returns NULL, no checks?

[frithjofh]

I agree, but localeconv() is only used to read a value, not to alter a value in any way. So I think other threads are not affected. Why would it not be thread save, where is the link I'm missing?

Your second thought about the function returning NULL I don't quite understand well. If localecon() would return NULL, what would have to happen for it to do so? I might think that if this happens the app itself has another problem already. But checking for return of NULL should not be a problem. I'll make an according change ...

Regards

frithjofh

[oBFusCATed]

http://www.cplusplus.com/reference/clibrary/clocale/localeconv/

The proper fix is probably to save the decimal separator in a variable at the beginning and then use the variable

[frithjofh]

at the beginning of where? the plugin? the entry of the function or the switch case? a const static variable of the function? what do you feel would be the best point?

I also have to admit that I don't like this function very much. According to the wxSplitterwindow reference the particular value should be between 0.0 and 1.0 both included, but it does not even say explicitly how many digits it should have, that is to say what stepping it accepts.

PS: about threads: any manipulation of the locale in another thread would change the result of localeconv() here. But the decimal separator always returns a char, if I understood right, under every possible locale, never an empty.

PS: perhaps a better way would be to parse the double into a wxString manually, composing the string by getting the integer value from the double, add a point, and the decimals from the double, all retrieved separately?

[oBFusCATed]

You can put it in the constructor of the plugin or the onactivate method. I'm not too familiar with the wxSmith's code.

[ollydbg]

@frithjofh

I suggest you put your patches in:
Features
Thus it never lost in the forum 

Thanks.

PS: I'm not familiar with wxsmith code.

[frithjofh]

@olldbg

Good idea. But I just went there, looked around, and am certain that any patch there will not be lost. I don't know if you know the movie "Indiana Jones - Raiders of the lost arc" . But it seems that any thing going into the berlios site under bugs, patches, etc will be "lost" the same way ...

And the patch is long from being acceptable I think, I think I'll just retire it from the forum, think it over, and post a bug report on berlios. 

thanks anyway for your interest 

frithjofh

PS: done

[oBFusCATed]

The patches section on the berios.de is the most visible one