On Sunday 03 of February 2019 14:27:18 Mike Bird wrote:

On Sun February 3 2019 03:55:00 Slávek Banko wrote:

...

Hello to all programmers,

please give your attention and share your opinion to the proposed patch for TQt3. For a detailed description of the problem, see the pull-request:

https://mirror.git.trinitydesktop.org/gitea/TDE/tqt3/pulls/6

Hi Slavek,

Sorry I don't have time to fully understand this code. All the levels of nested classes and shallow copying made my head spin but I don't see any obvious problems there. So just a couple of quick thoughts:

(1) I see a mutex and a lot of code that is not - at least to me - obviously thread safe.

As far as I know, all the mutex code is within the MAKE_TQSTRING_THREAD_SAFE condition, which is at the beginning of the file with the following comment:

// WARNING // When MAKE_TQSTRING_THREAD_SAFE is defined, overall TQt3 performance suffers badly! // TQString is thread unsafe in many other areas; perhaps this option is not even useful? // #define MAKE_TQSTRING_THREAD_SAFE 1

(2) Having the cstring in the TQStringData but the assignments to ctring in the TQString methods seems awkward but may be the best solution available.

TQStringData is an internal data structure for TQString. That's why there's nothing awkward about manipulating its items from TQString.

--Mike

Cheers

Fat-Zer wrote:

And IMHO this approach only relaxes coders and makes them fall for other pitfalls.

I second that, because you impact in both directions and how do we know that code using utf8() or local8Bit() works as intended after the change?

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512

On 2019/02/04 03:50 AM, Slávek Banko wrote:

Hi,

the purpose of the change is not to prevent bugs that programmers can do but to give the same scope for results from utf8() and local8Bit() as is for results from latin1() and ascii().

At the same time, the goal was to make the change so that there is no change in the public API.

The proposed change covers exactly these requirements. Nothing more, nothing less.

Cheers

Hi Alexander, Slavek, Slavek and I also don't like masking an issue and instead prefer to find the root cause whenever possible and practical. The problem that we have with the current code is that although utf8() and local8bit() return valid TQCString objects, it is extremely easy to run into problems when using the c_str() function of the return object in temporary expressions, like for example when it is necessary to pass a const char* to a function. The error is subtle and shows up at runtime instead of compile time, since syntactically the code is fine.

The solution proposed by Slavek makes the TQCString returned by utf8() and local8bit() persistent within the original TQString. Although this will solve 99% of the use cases, it stills suffers of the original problem where if the original TQString is a temporary object in a function call, the const char* would again point to an invalid object.

For R14.0.6 it makes somehow sense to go with Slavek's solution, to make things as less risky as possible without actually affecting the API. We should though open an Issue in gitea so that this problem is not forgotten. After R14.1.0 is out, we need to carefully consider various solutions and see which one is less intrusive and requires the least API changes, but at the same time fixes the problem for good.

Cheers Michele