[stdlib] Add more safeguards in the String constructors

[gabrieldemarmiesse]

Related to #2687

Fix #2392

Also related to #2678

Description of the problem:

EDIT: Please note that now, an empty string will allocate one byte on the heap for the null terminator. This will degrade the performance of some programs, the tradeoff is more safety. This performance penalty will go away with SSO, as the null terminator will be on the stack, which I hope can be implemented soon-ish when #2637 is fixed

[ConnorGray]

Note that this change means that constructing an empty string will now allocate, where it did not before. That may be what we want, but its a notable change to the default representation of a String.

E.g. a List[String] default initialized to empty String and then later filled in would now do O(N) allocations.

[gabrieldemarmiesse]

E.g. a List[String] default initialized to empty String and then later filled in would now do O(N) allocations.

Can you give a small code example? I'm not sure I see the regression you are talking about.

Pratically speaking, when we have SSO, empty strings will have null termintors allocated on the stack, so it will be very cheap. I'd advise that until then, we err on the side of caution about safety in String.

[ConnorGray]

That's a compelling point regarding the small-string optimization.

I'm convinced now that NUL terminating by default is not necessarily a bad behavior to aim for long term🙂

Can you give a small code example? I'm not sure I see the regression you are talking about.

Not a regression per se, but consider:

var list = List[String](String(), String(), String(), String(), String())

for count in len(list):
    list[count] = str(count)

Before this change, this program only allocates a String for each element once.

After this change, first five empty strings will be allocated, and then overwritten with the final content of the list as initialized by the for loop.

I don't necessarily think this is a convincing reason on its own not to move forward with this change. I'm just offering this as an example of the change in behavior this PR brings.

[gabrieldemarmiesse]

Thank you for the insight. Indeed it's a good example where we may have degraded performance. With SSO, that shouldn't be an issue. I'd be interested to know if a null terminator needs to be a requirement for String, but I guess this can be discussed after SSO is done

[ConnorGray]

One of the main benefits I can see from this change is that an empty String is now validly NUL terminated, so it can be passed to functions that are expecting a non-null string pointer.

So if thats the desired behavior, we should add some tests for that🙂

(But, I'd argue most functions that could accept an empty string should arguably be written to accept a null pointer, because as mentioned then they'll optimally allow the caller to avoid allocating space just to store an empty string.)

[gabrieldemarmiesse]

So if thats the desired behavior, we should add some tests for that🙂

Do you mean adding a test where if do assert_equal(String()._buffer[0], 0)?

EDIT: Nevermind, I understand now. I'll add the test tomorrow.

[gabrieldemarmiesse]

Done :)

[gabrieldemarmiesse]

I'll close this PR in favor of the SSO work. We can always see after SSO if this is still relevant.