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

reduce internal dependency on 0 terminated strings #5220

Merged
merged 1 commit into from Oct 23, 2015

Conversation

WalterBright
Copy link
Member

The idea is to eventually find and fix them all, then switch to D style strings. But finding them all is hard.

@yebblies
Copy link
Member

This is strndup right? Could you add xstrndup to root.rmem so we don't have to duplicate the memcpy + zero terminate pattern everywhere?

@WalterBright
Copy link
Member Author

The 0 termination can be more than one byte long. And I want to get rid of the 0 termination anyway.

@yebblies
Copy link
Member

Yes, but this pattern:

+            depmsg = cast(char*)mem.xmalloc(se.len + 1);
+            memcpy(depmsg, se.string, se.len);
+            depmsg[se.len] = 0;

is repeated quite a few times. And I assume there are more to come.

Another option would be to add a new member function to StringExp, which could be changed to return string when we're ready.

@WalterBright
Copy link
Member Author

I don't like strndup - I have a hard time remembering just what it does, and it likely has not received the intensive optimizations that memcpy has. I see no advantage to it.

Another thing I want to do to StringExp is have it always store its data as UTF8, and then convert to wchar or dchar only on demand. I have noticed latent bugs in dmd where it is assumed to be UTF8. Always storing it as UTF8 will eliminate those bugs and will make the logic much simpler to work with.

@WalterBright
Copy link
Member Author

But please pull this one first :-)

@andralex
Copy link
Member

So what I see here is this pattern repeated three times:

namez = cast(char*)mem.xmalloc(se.len + 1);
memcpy(namez, se.string, cast(size_t)se.len);
namez[se.len] = 0;

This screams for being put in a function. Either call strndup or write your own, but please don't commit such code duplication. Thanks.

@ibuclaw
Copy link
Member

ibuclaw commented Oct 22, 2015

Could you add xstrndup to root.rmem so we don't have to duplicate the memcpy + zero terminate pattern everywhere?

Which reminds me, we can remove those x prefixes now it's in D. Possible PR incoming...

This screams for being put in a function. Either call strndup or write your own, but please don't commit such code duplication.

I would concur here.

@WalterBright
Copy link
Member Author

done

final char* toStringz()
{
auto nbytes = len * sz;
char* s = cast(char*)mem.xmalloc(nbytes + sz);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto ...

andralex added a commit that referenced this pull request Oct 23, 2015
reduce internal dependency on 0 terminated strings
@andralex andralex merged commit 1fe51af into dlang:master Oct 23, 2015
@WalterBright WalterBright deleted the asciiz2 branch October 23, 2015 01:20
@yebblies
Copy link
Member

Thanks, this is much better.

@yebblies
Copy link
Member

@ibuclaw

Which reminds me, we can remove those x prefixes now it's in D. Possible PR incoming...

Don't they still need to be called from the glue layer?

@ibuclaw
Copy link
Member

ibuclaw commented Oct 23, 2015

Don't they still need to be called from the glue layer?

If I need to allocate memory, there's gcc gc. The problem was that when you (or someone) changed around the Array implementation to Array, suddenly rmem.h was being pulled into places it wasn't welcome.

I reckon that the uses of rmem exposed aren't necessary if allocation is kept in the frontend.

@yebblies
Copy link
Member

Array still uses rmem, and is still used from the glue layer in dmd.

@9rnsr
Copy link
Contributor

9rnsr commented Nov 21, 2015

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=15369

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