JS_GetClass function

[caturria]

Hi,

Thanks again for this fantastic library!

I would like to add a JS_GetClass function so that the host application can inspect the details of native classes. Useful especially for generation of context-specific error messages that include class names.

Are there any concerns regarding the definition of JSClass being moved to the public header?

I would also like to disclose that I am totally blind. Though I tried to match the style as best I could, your feedback on any stylistic inconsistencies I may have missed would be greatly appreciated.

Thank you.

[bnoordhuis]

Exposing structs in the public API effectively freezes them; we can no longer add, remove, rename or reorder fields without breaking backwards compatibility. We're not at a stage yet where we actually promise API stability but eventually we will be.

A better approach is to add something like JSAtom JS_GetClassName(JSRuntime *rt, JSClassID class_id). Returning a JSAtom is perhaps sub-optimal because:

1. the fact the class name is stored as a JSAtom is an implementation detail, and
2. most callers presumably are going to turn it into a string anyway

Therefore int JS_GetClassName(JSRuntime *rt, char *buf, int len) is maybe better still, but I'm open to being convinced otherwise.

[caturria]

I'm happy to do whatever you tell me to. I had considered the atom approach as well. There was a JSClass typedef in quickjs.h that was never actually used, so I thought perhaps it was meant to be there but forgotten or some such.

To me, the downsides to the int JS_GetClassName(JSRuntime *rt, JSClassId id, char *buf, int len) approach are an extra memcpy and an extra failure case for the caller to defend against (being told the provided length is insufficient).

I personally like JSAtom JS_GetClassName(JSRuntime *rt, JSClassID id) because this allows the caller to choose whether to convert it to a cstring or to a JSValue string without having to pay for an extra copy from one to the other.

Another option is const char *JS_GetClassName(JSContext *ctx, JSClassID id) with a note that the caller must JS_FreeCString it, but if the caller intended to convert it to JSValue then they pay for an unnecessary copy to cstring first.

[bnoordhuis]

@saghul your API design thoughts, if you please?

[chqrlie]

I like the JSAtom JS_GetClassName(JSRuntime *rt, JSClassID id) approach.

[harumazzz]

Agreed. I think the approach return JSAtom is much nicer.

[bnoordhuis]

Okay, JSAtom it is, then.

@caturria make sure to JS_DupAtom the return value, and document that the caller must call JS_FreeAtom.

[saghul]

Chiming in late... we do have other APIs what return names like the module name and script name for workers, and they return JSAtom, so there is that!

[caturria]

Okay. Done.

[bnoordhuis]

Style issues but otherwise LGTM.

[bnoordhuis]

Suggested change

	JSAtom JS_GetClassName(JSRuntime *rt, JSClassID id)
	{
	if(!JS_IsRegisteredClass(rt, id)) {
	return JS_ATOM_NULL;
	}
	JSAtom ret = rt->class_array[id].class_name;
	JS_DupAtomRT(rt, ret);
	return ret;
	}
	JSAtom JS_GetClassName(JSRuntime *rt, JSClassID class_id)
	{
	if (JS_IsRegisteredClass(rt, class_id)) {
	return JS_DupAtomRT(rt, rt->class_array[class_id].class_name);
	} else {
	return JS_ATOM_NULL;
	}
	}

[caturria]

Github wouldn't let me directly commit the suggestions for whatever reason. I think I got 'em all?
Once again thanks for your patience with this. As a blind person especially whitespace-related inconsistencies are very difficult to even notice without proofreading letter-by-letter and even then they're easy to miss.

[bnoordhuis]

There are still some left but I'll fix them manually when I merge your PR.

[bnoordhuis]

Suggested change

	JS_EXTERN JSAtom JS_GetClassName(JSRuntime *rt, JSClassID id);
	JS_EXTERN JSAtom JS_GetClassName(JSRuntime *rt, JSClassID class_id);

[caturria]

Thank you.

[saghul]

Style, spacing

[bnoordhuis]

Merged with whitespace fixes in 307a59f, thanks!