jamoma / JamomaCore

Jamoma Frameworks for Audio and Control Structure
Other
36 stars 14 forks source link

TTString: Assess moving back to a type alias of std::string #339

Open tap opened 9 years ago

tap commented 9 years ago

At some point in the past TTString was a typedef of std::string. Life was good.

We had super difficult problems on Windows and based-on nebulous information floating around the internet we decided the memory corruption could be coming from TTString as it crossed DLL boundaries -- perhaps std::vector would be safer because the storage memory internally is guaranteed to be contiguous according the C++ standard itself.

As a result of the above work the problem moved elsewhere. Which proves nothing. And we still had a problem. In fact, there is a no guarantee when using different compilers or compiler settings about passing any of the STL containers across DLL boundaries. That means the current TTString isn't really safer than the old TTString -- it's just different. Laurent rewrote based on C-strings and did manage to stop the crashes. For reference: the email thread for the above is called "Jamoma on Windows" on the jamoma-devel list.

And furthermore, as @jcelerier wrote to jamoma-devel ( "Regarding TTString" ), the current C++ standard (section 21.4.1) states that "The char-like objects in a basic_string object shall be stored contiguously".

We have three options (at least):

  1. leave it as is, std::vector, which is a hassle for maintenance and not feature complete and buys us nothing
  2. move to std::string, which means a few bits of code will need to change where we added custom extensions like the random() method
  3. re-implement from scratch in C, but see below

My ( @tap ) assessment:

If people use difference compilers and compiler settings for different libs I suspect they are going to be totally hosed. There are lots of places (not just TTString) where memory layout and such is going to become a problem. Unless we want to rewrite all of Jamoma with a C API then we need to accept that all of Jamoma must be compiled with the same compiler and compiler settings to be used.

Therefore, we should move back to std::string.

tap commented 9 years ago

One dangling question needs to be resolved: it does not appear that Laurent's work was ever merged.It is on a branch called "windows-string-rewrite3-cstring". But Windows is now working correctly on the dev branch with the vector TTString if I understand correctly. Do I understand correctly?

jcelerier commented 9 years ago

On Mon, Mar 2, 2015 at 2:43 PM, Timothy Place notifications@github.com wrote:

But Windows is now working correctly on the dev branch with the vector TTString if I understand correctly. Do I understand correctly?

Yes.

About the problem here, as far as I know, on Windows passing C++ stuff across DLL boundaries only works if both the DLL and the thing that calls / load the DLL were built against the same C runtime version, mostly because MS's implementation often changes (for instance I think std::string was using copy-on-write some years ago, but not anymore, which would of course immediately crash if a non-COW string had a COW method called on it).

To "solve" this headache, Qt release binaries built against the two or three last versions of MSVC (2010, 2012, 2013 right now : http://www.qt.io/download-open-source/#section-3 ) and people have to rebuilt it from source on their own if they want to use another version for their app...


Jean-Michaël Celerier http://www.jcelerier.name

tap commented 9 years ago

Note: before we close this ticket out we should also prune these old branches from the repository:

remotes/origin/windows-string-rewrite remotes/origin/windows-string-rewrite2 remotes/origin/windows-string-rewrite3-cstring

tap commented 9 years ago

There are a couple of places where we will have changes in behavior as a result of this. So we are unfortunately not going to be 100% backward compatible. Nevertheless, the benefits of being standards-compliant outweigh the cost of some short term pain on edge cases that may or may-not exist.

Here is one example (I had to modify our unit tests to pass): TTString s = "pi is roughly "; s += 3.14; This code readily compiles but does not produce "pi is roughly 3.14" as the result. The C++11 standard way of doing this is: TTString s = "pi is roughly "; s += to_string(3.14); Rather than overriding the standard way std::string implements += we should just use std::to_string(). Hopefully there aren't too many places lurking in our code that do this.

A second place where there could be edge cases (I once again had to modify our unit tests to pass) is in number elements used to store a string. In our custom-coded std::vector derivative we stored the NULL termination in an extra byte. Again, hopefully there aren't too many places this will impact.

One place it has impacted heavily is in weird NodeLib code which had to work around the NULL termination byte. So I've been needing to remove those hacks now. They were caught by unit tests -- any cases not caught by unit tests will be a problem however...

nwolek commented 9 years ago

This looks like something we would do quite frequently in the unit tests themselves, right? I am thinking of how we post things to the console.

-Nathan

On Mar 3, 2015, at 1:45 PM, Timothy Place notifications@github.com wrote:

Here is one example (I had to modify our unit tests to pass):

TTString s = "pi is roughly "; s += 3.14;

This code readily compiles but does not produce "pi is roughly 3.14" as the result. The C++11 standard way of doing this is:

TTString s = "pi is roughly "; s += to_string(3.14);

Rather than overriding the standard way std::string implements += we should just use std::to_string(). Hopefully there aren't too many places lurking in our code that do this.