-
Notifications
You must be signed in to change notification settings - Fork 56
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
Drop ctx argument from functions that don't need it #156
Conversation
JS_NewInt32 and js_int32 are the same, what is the reasoning for having "duplicate" implementations? Is it the force-inline? |
Shall we drop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a question.
No, it's purely stylistic / unclutter. If we're okay with backwards-incompatible API changes, or versioning the API somehow, we could make the change directly to quickjs.h.
Good point! I was meaning to eradicate the two remaining calls from quickjs.c but I forgot. I'll fix that. As to your general question: it serves no purpose so... yes? |
746729c
to
87021c0
Compare
I am somewhere near +0.5, I guess if we are doing a new thing might as well drop the baggage? OTOH, having existing compile as-is looks like a big plus, so let's have it! |
I'll go ahead and merge this and then ponder for a bit whether or not to change the public API. |
Drop the unused JSContext argument from several oft-used functions.
Non-functional change (
strip build/qjs && ls -l build/qjs
is the same before and after) but improves legibility, IMO. No change to the public API either.