No more warning popup when using a math function with a semicolon

[Onetchou]

solves #1153

[what-the-diff]

PR Summary 📝

• Enhancement of qmuparsertokenreader.cpp File 🚀

  Our superstar developer improved the file qmuparsertokenreader.cpp. Changes include the addition of a resizing function, specifically devised to adjust the szSep QString to a precise size of 2. This helps us manage the capacity of szSep in a more effective way! 😄

  Moreover, the new update designates the value of m_cArgSep to be the first element of szSep. This works towards improving the arrangement of elements in szSep, making our code cleaner and easier to understand. 🎉

  Lastly, our developer set the second element of szSep to the value 0. It's a minor but important change that contributes toward smoother and efficient functioning of the szSep QString element. 🌟

So, in a nutshell, this PR is all about superior control and enhanced performance. Excellent job to our dev team for making these thoughtful improvements! 🥳

[DSCaskey]

I'm approving this as there's down side to it, but I'd like to know what the source of the orginal issue is and why this fixes it, as I can't reproduce the error with MSVC 64bit builds in my locale.

[Onetchou]

I'm approving this as there's down side to it, but I'd like to know what the source of the orginal issue is and why this fixes it, as I can't reproduce the error with MSVC 64bit builds in my locale.

Mmmm, strange that you don't have the same warning...

It seems that doing so is deprecated because when the code reaches the 3rd line, szSep is equal to ";" and has a length of 1. When calling szSep[1] = 0;, 1 is out of range:

QString szSep;
szSep[0] = m_cArgSep;
szSep[1] = 0;

The Qt documentation recommends using resize after the string declaration to initialize it character by character:

[DSCaskey]

Mmmm, strange that you don't have the same warning...

I checked a slew of builds... old Seamly 32 & 64 bit gcc builds, my current Seamly MSVC 2022 build, a current Seamly MSVC 2014 release... even some Valentina builds... none of them produce the error. The question is.. are you getting the error only with your builds? or with any downloaded Seamly releases too? If it's just your builds it has to be something in your build environment. If it's any build, then it might be your machine or locale... in which case other users may also be affected. To be honest most users are not going to be using the math functions at all. I don't know of any pattern system requires them... they're just there because they were in the orginal muparser lib.

QString szSep;
szSep[0] = m_cArgSep;
szSep[1] = 0;

It seems that doing so is deprecated because when the code reaches the 3rd line, szSep is equal to ";" and has a length of 1. When calling szSep[1] = 0;, 1 is out of range:

Not sure what you mean when you say 1 is out of range? It looks pretty clear that szSep's 1st char is set to ';', its second char is set to 0 (null). A QString will grow in size if needed. Yes you can set the resize of the string to 2, but that's an efficency thing so as not to allocate more memory than neccessary. So I don't lnow if it's a locale issue, a utf issue, or something in your build configuration that's doing something to the QString.

Keep in mind that the qmuparser is a port to Qt by RT of the muparser. So he didn't change any of the odd coding - including all the annoying spaces before and after parenthesis. That being said... a simpler, clearer, and more Qt way:

// copy the separator into null terminated string
QString szSep = m_cArgSep + QChar::Null; // Makes it clear the last char is null. We don't need to know the len of szSep
                                                                     // We also should not need the preceding comment to say we're null terminating 
                                                                     // the string as QChar::Null should make that clear.  szSep[1] = 0... not so clear.

Could we do this to save memory?

QString szSep;
szSep.resize(2);
szSep = m_cArgSep + QChar::Null;

Sure... but worrying about this instance is trival... like worrying about how many tool ids we can create before running out. szSep is only going to be inscope for a few millisecs.

[Onetchou]

The question is.. are you getting the error only with your builds? or with any downloaded Seamly releases too?

Only with my builds indeed.

It looks pretty clear that szSep's 1st char is set to ';', its second char is set to 0 (null). A QString will grow in size if needed.

I'm not sure about that. The [] operator is only "return[ing] the character at the specified position in the string as a modifiable reference.", I'm not aware of it being able to increase the qstring size. There are other methods to append chars to strings. This operator does not check if the given index is out of range or not, leading to a random behavior that seems to depend on the configuration if a out-of-range index is given.

QString szSep; is creating a null string of size 0, thus both 0 and 1 are out of range. I can't find a reliable source explaining that accessing a null string with a [n], $n \in N$, is ok.

[DSCaskey]

I'm not sure about that. The [] operator is only "return[ing] the character at the specified position

Right... if you are ACCESSING not assigning. szSep[1] = 0; is not accessing anything... it's assigning the 2nd char the value 0. On the other hand char3 = szSep[2} would be invalid as it's trying to access a 3rd char that does not exist in the string as the string length = 2. Well it exists - as a memory address. but it's invalid data.

I'm not aware of it being able to increase the qstring size.

I modified the tooltip example to illustrate:

Where it shows that the length of the string grows as you assign new chars by indexing the array. Which is exactly what the math parser code is doing without needing the resize.

Only with my builds indeed.

Which is a good thing, but like I said... it's more an issue with your configuration, not the code. What that is I can only speculate, but it would be nice to know.

[Onetchou]

Right... if you are ACCESSING not assigning. szSep[1] = 0; is not accessing anything... it's assigning the 2nd char the value 0.

I may be wrong, but I think that by writing szSep[1] you are still accessing the character at index 1, which is not part of the QString, and szSep[1] is returning a modifiable reference to this character, and then, by adding the =0 you're modifying its value.

Qt seems to understand what you're trying to do and is increasing the size of the QString accordingly, but this seems to be deprecated, as mentioned in the warnings:

I modified the tooltip example to illustrate:

I get that it's currently working, but it doesn't seem to be a "clean" way of doing it. I can't find any official Qt doc showing this way of appending chars to QStrings.

Using the append method or pre-allocating the string with the resize seem to be better ways of doing it 😉

Only with my builds indeed.

Which is a good thing, but like I said... it's more an issue with your configuration, not the code. What that is I can only speculate, but it would be nice to know.

Since this way of appending a char to a qstring is known to be deprecated and to cause random behaviors, it's partly the code's fault if the behavior depends on the computer's configuration...