Skip to content

Conversation

@Krovatkin
Copy link
Collaborator

This fix makes sure that AutoString::length and AutoString::data are in sync.

@Krovatkin Krovatkin changed the title Update length to match char* data Keep AutoString::length in sync w/ AutoString::data Aug 25, 2017
return errorCode;
}

size_t GetLength()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AutoString::GetLength is used only in Debugger.cpp (namely, https://github.com/Microsoft/ChakraCore/blob/master/bin/ch/Debugger.cpp#L497). If ChakraFull doesn't have any additional dependencies we could do away with AutoString::GetLength

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Cellule what do you think?

@obastemur
Copy link
Collaborator

This supposed to cache string length so we don't measure it again and again.. Well, this is not perf critical part of the solution.

LGTM. Yet, could we do it while caching the length ?

@Krovatkin
Copy link
Collaborator Author

@obastemur I pondered a bit on the caching aspect. Unfortunately, this design created enough complexity to produce a bug. Moreover, some uses in WScriptJsrt.cpp still call strlen. All the uses of AutoString seem to be contained in WScriptJsrt.cpp and Debugger.cpp and neither one seems to be perf critical. Lastly, users of AutoString should be able to cache the result of GetLength in most cases in case we ever hit a perf issue.

Anyways, I'll do whatever you guys tell me to.

fyi @arunetm

@Cellule
Copy link
Contributor

Cellule commented Aug 25, 2017

@Krovatkin First of all it won't affect ChakraFull since this file is in ch.exe only

I found the source of the bug.
in commit 356656c (aka #3433). the length used to be cached from the ChakraRTInterface::JsCopyString call in AutoString::Initialize. Instead now it is using a local variable instead thus breaking the initial contract of AutoString.

I think we have to use AutoString::length instead of writtenLength in AutoString::Initialize (both CopyString calls) and that should be the extent of the fix.

Thoughts ?

@Krovatkin
Copy link
Collaborator Author

@Cellule, done

@Krovatkin
Copy link
Collaborator Author

@dotnet-bot test Ubuntu static_ubuntu_linux_release please

@Cellule
Copy link
Contributor

Cellule commented Aug 25, 2017

Thanks @Krovatkin, I will merge it for you

@chakrabot chakrabot merged commit d64b4ce into chakra-core:master Aug 25, 2017
chakrabot pushed a commit that referenced this pull request Aug 25, 2017
…g::data

Merge pull request #3588 from Krovatkin:autostr_len_fix

This fix makes sure that `AutoString::length` and `AutoString::data` are in sync.
@Krovatkin
Copy link
Collaborator Author

@Cellule thank you for your help!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants