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

portability: global structs cant be statically initialized with external symbols #3

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

trufae
Copy link

@trufae trufae commented Dec 30, 2022

No description provided.

Copy link
Member

@oleavr oleavr left a comment

Choose a reason for hiding this comment

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

Thanks!

quickjs.c Show resolved Hide resolved
static const JSCFunctionListEntry js_math_funcs[] = {
JS_CFUNC_MAGIC_DEF("min", 2, js_math_min_max, 0 ),
JS_CFUNC_MAGIC_DEF("max", 2, js_math_min_max, 1 ),
JS_CFUNC_SPECIAL_DEF("abs", 1, f_f, fabs ),
JS_CFUNC_SPECIAL_DEF("floor", 1, f_f, floor ),
Copy link
Member

Choose a reason for hiding this comment

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

Sorry just noticed one thing that I missed last time 😓 Is there a reason floor() and ceil() are wrapped but not fabs(), acos(), etc.?

Copy link
Author

Choose a reason for hiding this comment

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

the compiler didnt complained about those.. for some unknown reason

@@ -42193,14 +42193,29 @@ static JSValue js_math_random(JSContext *ctx, JSValueConst this_val,
return __JS_NewFloat64(ctx, u.d - 1.0);
}

static double qjs_ceil(double x)
Copy link
Member

Choose a reason for hiding this comment

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

Looking closer at the QuickJS style, it looks like we should fix two inconsistencies here:

  • namespace these like the other functions here, e.g. js_math_ceil() instead of qjs_ceil()
  • make sure the function definitions are in the same order here as they are down in js_math_funcs

Copy link
Author

Choose a reason for hiding this comment

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

uhm not really. the js_* functions are javascript functions. what im wrapping here is libc functions

Copy link
Author

Choose a reason for hiding this comment

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

oh well. seems like js_math is also used to wrap c functions. so its a bit inconsistent. but ill follow their inconsistency

Copy link
Author

Choose a reason for hiding this comment

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

Actually the convnetion is just prefix them with js_ because the direct calls are accessed thru the f_f and f_f_f functions and those are the ones that handle the js context instead

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.

3 participants