Code cleanup #132
Merged
Code cleanup #132
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Splits longish v8js.cc file into pieces. It used to contain the definitions of all V8Js exception classes, the V8Js class itself as well as the V8Object/V8Function classes... besides the module setup code itself. This change factors out all exception class definitions into a seperate pair of files (v8js_exceptions.*). The V8Js class definition itself is moved to v8js_class.* pair, with the v8 init & call code moved to v8js_v8.* pair and the watchdog timer to v8js_timer.* pair. The V8Object/V8Function code was moved to v8js_v8object_class.* pair, not differentiating between the two like before.
* drop php_ prefix, currently used inconsistently anyways * change "object" to "v8object" to clarify it's a JS object wrapped to a V8Object instance
|
👍 have you reviewed this at all @cscott ? |
|
@stesie do you still want this merged? It looks good to me. |
|
I'd still like to merge this, but haven't felt the urgent need to do so without feedback. I'm currently spending some time off V8Js and besides a bit stuck on the other cleanup ticket (#72); hence I probably would have followed up here if the other patches are ready. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a major code cleanup I'd like to propose before tackling issue #72 which is going to introduce new classes and a trait.
Currently the v8js.cc file is rather messy IMHO as it not only holds everything related to our V8Js main class, but also V8Object & V8Function classes as well as all exception classes + v8 initialization.
I've now split all that stuff into seperate files (v8js_class.cc, v8js_v8object_class.cc, v8js_exceptions.cc + non class stuff in v8js_timer.cc & v8js_v8.cc)
Besides I found the similarly named handler functions relating to v8js (php_v8js_XXX) and v8object (php_v8js_v8_XXX) classes rather confusing and hence renamed the latter to v8js_v8object_XXX (i.e. with "object" term in it)
Also we had a huge mess with symbols beginning with "php_v8js_" and just "v8js_"; e.g.
php_v8js_ctx,php_v8js_execute_stringas opposed tov8js_to_zval,v8js_persistent_obj_tet al.So I removed all the leading php_ prefixes.
@preillyme @cscott please let me know what you think about that change; if you agree or if I go too far (especially with the renaming of existing symbols, not just moving them around)
cheers
~stesie