Skip to content

Conversation

@Cellule
Copy link
Contributor

@Cellule Cellule commented Apr 12, 2016

Relax assert in Globalization IsWellFormed. It is valid to have nullptr as a string as it will correctly throw a javascript Error that the Local is not well-formed.


This change is Reviewable

@Cellule
Copy link
Contributor Author

Cellule commented Apr 12, 2016

@akroshg Can you review this please ?

@akroshg
Copy link
Contributor

akroshg commented Apr 12, 2016

looks good to me.

It is valid to have nullptr as a string as it will correctly throw a javascript Error that the Local is not well-formed.
// will be rejected by globalization dll
IfFailThrowHr(GetWindowsGlobalizationLibrary(scriptContext)->WindowsCreateStringReference(languageTag, static_cast<UINT32>(wcslen(languageTag)), &hStringHdr, &hString));
AnalysisAssert(hString);
if (hString == nullptr)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this because Prefast wouldn't let it pass.
For some reason the function IsWellFormed doesn't allow null in the signature, but supports it.

@Cellule
Copy link
Contributor Author

Cellule commented Apr 12, 2016

@akroshg is my additional change ok ?

@akroshg
Copy link
Contributor

akroshg commented Apr 12, 2016

Yes - infact this is better as we are future proofing ourselves in the case where IsWellFormed could not handle null itself.
LGTM

@chakrabot chakrabot merged commit 6535b1f into chakra-core:release/1.2 Apr 13, 2016
chakrabot pushed a commit that referenced this pull request Apr 13, 2016
Merge pull request #780 from Cellule:globalization
Relax assert in Globalization IsWellFormed. It is valid to have nullptr as a string as it will correctly throw a javascript Error that the Local is not well-formed.
chakrabot pushed a commit that referenced this pull request Apr 13, 2016
Merge pull request #780 from Cellule:globalization
Relax assert in Globalization IsWellFormed. It is valid to have nullptr as a string as it will correctly throw a javascript Error that the Local is not well-formed.
@Cellule Cellule deleted the globalization branch April 13, 2016 02:07
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.

4 participants