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

Restructure lib/Runtime/Library directory #6709

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

MadProbe
Copy link
Contributor

@MadProbe MadProbe commented Apr 19, 2021

I am not sure if i moved all files to their folders correctly, so if you, @rhuanjl, have any suggestions to move something, i'll apply your changes without hesitation.

@ppenzin, Here is the changes:
I added 8 folders with self-descriptive names: Array, Functions, Generators, Iterators, JSON, Regex, String, and WASM

@rhuanjl
Copy link
Collaborator

rhuanjl commented Apr 19, 2021

I'll look at this in more detail later; though quick note - ignore the CI copyright check error.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Apr 19, 2021

@ppenzin interested what you think here as well - simply an attempt to make the lib/Runtime/Library folder a little easier to navigate with some new subfolders.

See index here: https://github.com/MadProbe/ChakraCore/tree/structurization/lib/Runtime/Library vs current master.

Of the new subfolders they all seem fairly self-explanatory and sensible to me other than perhaps the Functions one.

I also wonder if we should also move the following files (all of which are used for the self-hosted JS library methods) into the InJavascript folder:

IntlEngineInterfaceExtensionObject.cpp
IntlEngineInterfaceExtensionObject.h
IntlExtensionObjectBuiltins.h
EngineInterfaceObject.cpp
EngineInterfaceObject.h
EngineInterfaceObjectBuiltIns.h
JsBuiltInEngineInterfaceExtensionObject.cpp
JsBuiltInEngineInterfaceExtensionObject.h

Copy link
Member

@ppenzin ppenzin left a comment

Choose a reason for hiding this comment

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

@ppenzin interested what you think here as well - simply an attempt to make the lib/Runtime/Library folder a little easier to navigate with some new subfolders.

I like it, I always felt that Runtime/Library has grown a bit too big.

I also wonder if we should also move the following files (all of which are used for the self-hosted JS library methods) into the InJavascript folder

What InJavascript directory supposed to be for - for builtins' sources and generated bytecode or everything builtin-related? Maybe they should go there, or maybe it needs one more level (to distinguish between C++, JS, and generated code). I don't have a strong opinion on this, there is a number of good ways to do it.

Minor nit: I'd use Function instead of "Functions" (I'm fine either way, this is not significant).

@MadProbe, ror commit readability can you please add the list of new directories - either in the comments here or into one of the commits.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Apr 21, 2021

Looking at the proposed Functions folder some of the things in it are somewhat different things in particular:

  • the 4 ArgumentsObject files support the use of the arguments keyword they're not innate to functions
  • The 3 JavascriptBuiltInFunction are providing a list of library methods for the Jit to reference

@pleath
Copy link
Contributor

pleath commented Apr 21, 2021

Is it at all an issue for you that a broad restructuring change will make it difficult to propagate changes from released MSFT branches to master? This includes CVE fixes and any outstanding feature work, like the change I've been holding onto to share types with deleted properties.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Apr 21, 2021

Is it at all an issue for you that a broad restructuring change will make it difficult to propagate changes from released MSFT branches to master? This includes CVE fixes and any outstanding feature work, like the change I've been holding onto to share types with deleted properties.

This is a valid point - I was the under the impression that we weren't expecting any significant further MSFT input. But perhaps it's worth delaying this for a little longer

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