-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add Snake to Camel Case and Camel to Snake Case #737
Add Snake to Camel Case and Camel to Snake Case #737
Conversation
Looks as though it's still having a little bit of trouble tracking the memory on this one, I'll see if I can figure out what is happening later on today then get this one reviewed for you! |
This is a classic case of "worked on my machine". I pushed it up and work got crazy. I've been trying to get back to it. :/ |
Haha yeah I’ve been pretty busy too! Honestly no sweat tho, I appreciate any time you get and of course life comes first! |
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
3527781
to
7c8a820
Compare
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
Apologies been inactive on this, I'm actually on holiday for 5 weeks but I promise I will get around to it!! |
No worries. I got the memory issue resolved and need to figure out the Mac issue now. Been super busy with work but I think I can wrap this up this week. Thanks! |
Mac issue should be sorted now! I’ll take a look at the memory issue sometime tomorrow hopefully. I’ve been a bit inactive on this and need to get back into it, apologies |
It's insanely coincidental that you looked at this as just last night I did as well but it's been awhile and my Mac has updated so I'm fighting through getting the project to build again. What was the fix? |
The mac actions issue was the fact that -latest is now on an ARM processor so the FFI module was failing, but @liz3 sorted that out for us! |
Very strange happenings on Mac:
The very odd occasion I get erroneous output:
|
int i, j; | ||
for (i = j = 0; i < string->length; i++, j++) { | ||
if (string->chars[i] == '_') { | ||
temp[j] = toupper(string->chars[i+1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super dangerous, it does assumptions about the length of string, if a string ends with "_" this will append the null byte as content to temp, if toupper handles that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah valid point, probably need a length check here
ObjString *string = AS_STRING(args[0]); | ||
|
||
if (string->length < 3) { | ||
return OBJ_VAL(string); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a missmatch in behaviour, because the later implementation returns a new string, this returns a pointer to the existing string if i am not mistaken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not the end of the world, strings are interned in the VM anyways
i do think the
produces
which makes sense since we need to set the length index for the 0 byte. You can even see it:
(and yes |
Yep that was it 🤦 it should have been j not |
Gonna close this out for now with the new UTF8 strings coming in, but happy for this method to be re-added |
Im thinking this is maybe something which could be implemented in dictu itself, the reason being that some functionality might not be used enough no need the speedup in c. Comparing to js/python here. |
Add Snake to Camel Case and Camel to Snake Case
What's Changed:
Added 2 methods to the string type.
Type of Change:
Housekeeping:
Screenshots (If Applicable):