Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
tap opened this issue Mar 2, 2015 · 5 comments
Open

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

tap opened this issue Mar 2, 2015 · 5 comments
Assignees

Comments

@tap
Copy link
Member

tap commented Mar 2, 2015

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
Copy link
Member Author

tap commented Mar 2, 2015

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?

@tap tap added this to the Foundation Code Review milestone Mar 2, 2015
@jcelerier
Copy link
Member

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
Copy link
Member Author

tap commented Mar 2, 2015

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 tap added the in progress label Mar 3, 2015
@tap tap self-assigned this Mar 3, 2015
@tap
Copy link
Member Author

tap commented Mar 3, 2015

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
Copy link
Member

nwolek commented Mar 4, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants