Skip to content

Conversation

@jackhorton
Copy link
Contributor

This has been bothering me for a while. This also reduces the size of ChakraCore.dll by 16KiB by removing a bunch of duplicate EntryInfos

@jackhorton jackhorton added Dev Experience Bytecode-Update This PR updates bytecode and will cause merge conflicts with other PRs with this label labels Sep 9, 2018
ENTRY(builtInGlobalObjectEntryIsNaN)
ENTRY(builtInGlobalObjectEval)
ENTRY(builtInJavascriptArrayEntryConcat)
ENTRY(builtInJavascriptArrayEntryIndexOf)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these 3 special and not included in the header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because the header goes hand in hand with installing JavascriptArray::EntryInfo::IndexOf and the like onto the engine interface object, but since theyre jsbuiltins, the jsbuiltin interface object goes and basically monkey-patches it (if I remember correctly). @sigatrev would know more.

@jackhorton jackhorton force-pushed the engineinterface-cleanup branch from 397c46e to 3c50600 Compare September 13, 2018 18:30
@jackhorton jackhorton force-pushed the engineinterface-cleanup branch from 3c50600 to 650528b Compare September 14, 2018 18:26
@chakrabot chakrabot merged commit 650528b into chakra-core:master Sep 15, 2018
chakrabot pushed a commit that referenced this pull request Sep 15, 2018
…bject

Merge pull request #5676 from jackhorton:engineinterface-cleanup

This has been bothering me for a while. This also reduces the size of ChakraCore.dll by 16KiB by removing a bunch of duplicate EntryInfos
* @param {String} key the specific key we are looking for in the extension, such as "co"
*/
const UnicodeExtensionValue = function (extension, key) {
raise.assert(key.length === 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

raise.assert(key.length === 2); [](start = 8, length = 31)

was this assert not valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

meh -- the spec has lots of lines like "assert x equals some condition." this was intended to replicate that, but its not clear what value it has (its not clear to me what error we should throw on such an assert or if we should failfast, and we already have numerous failfasts in the internal c++ bits.

@dilijev
Copy link
Contributor

dilijev commented Sep 19, 2018

FWIW LGTM

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

Labels

Bytecode-Update This PR updates bytecode and will cause merge conflicts with other PRs with this label Dev Experience

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants