Implement Value.TypeOf() #104

stroiman · 2025-05-10T12:04:37Z

I mostly wanted to have Value.TypeOf() to help inspecting values returned when implementing ESM - so I could just as well implement it as a separate PR.

I did some refactor to move cleanup code to the C-side.

On the go side, I changed the := declare and assign to var with explicit types for how it reads, as I find it becomes more obvious on the next line that the cleanup code corresponds to the value.

But I also created cleanup helpers for this, as well as the Error type.

	var s C.RtnString = C.ValueToString(v.ptr)
	defer C.RtnStringRelease(s)

I also created some helper functions to convert both an error and a Local<String> to this value, but I actually thought that this type was used in more places. Turns out that there are only two, as the ValueToString doesn't actually use a Local<String>.

But some of the logic has more reuse potential because the type here is for the case where there is both a string and error value returned. There are more places where it's just a string value returned and no error, so the general conversion of a Local<string> to size_t/char* pair can be used in more places. E.g., I have some in my ESM branch, so I might make a refactor PR on this alone to simplify transfering Local<String> to go code.

stroiman · 2025-05-10T12:13:09Z

Btw, an alternate implementation could involve a small unexported helper type on the Go side.

While writing this, I see the only place that actually may return an error from the C side is DetailString but that actually panics on the Go side, so it appears that this type RtnString that has both a string and an error is an odd one out, and I'm even more inclined to extract the string handling alone.

type v8StringAndError C.RtnString

func (s v8StringAndError) release() { C.RtnStringRelease(s) }
func (s v8StringAndError) String() string { return C.GoStringN(s.data, C.ing(s.length) }
func (s v8StringAndError) retval() (string, error) { 
	if rtn.data == nil {
		err := newJSError(rtn.error)
		return nil, err
	}
    return C.GoStringN(s.data, C.ing(s.length), nil
}

func (v *Value) TypeOf() string {
	s := v8StringAndError(C.ValueTypeOf(v.ptr))
	defer s.release()
	return s.String()
    // Or when there is the possibility of an error
    // return s.retval()
}

stroiman · 2025-05-10T12:16:13Z

Regarding DetailString, the C header docs read

  /**
   * Provide a string representation of this value usable for debugging.
   * This operation has no observable side effects and will succeed
   * unless e.g. execution is being terminated.
   */
  V8_WARN_UNUSED_RESULT MaybeLocal<String> ToDetailString(
      Local<Context> context) const;

So a panic on the Go side seems reasonable.

tommie

Indeed a nice cleanup!

tommie · 2025-05-21T07:38:47Z

value.go

@@ -240,8 +240,8 @@ func (v *Value) Object() *Object {
 // are returned as-is, objects will return `[object Object]` and functions will
 // print their definition.
 func (v *Value) String() string {
-	s := C.ValueToString(v.ptr)
-	defer C.free(unsafe.Pointer(s.data))
+	var s C.RtnString = C.ValueToString(v.ptr)


Normally := is preferred. Why this change?

tommie · 2025-05-21T07:40:59Z

errors.cc

@@ -48,3 +49,15 @@ RtnError ExceptionError(TryCatch& try_catch, Isolate* iso, Local<Context> ctx) {

  return rtn;
 }
+
+void ErrorRelease(RtnError err) {
+  if (err.msg) {


free on a nullptr is a no-op, so you don't need this check. Same further down.

I didn't know this - nice to get rid of this.

tommie · 2025-05-21T07:41:54Z

errors.cc

+
+void ErrorRelease(RtnError err) {
+  if (err.msg) {
+    free((void*)err.msg);


Casting to void* seems unnecessary?

I would have through so as well

And I also get that error running go test

What's the error?

Edit removed wrong reference.

(I don't know about your editor. Seems like it doesn't know how C works.)

If I remove the (void*) I get the same error message building, "no matching function for call to 'free'" (I mentioned go test because it just runs automatically on save - but it is a build error)

❯ go build # github.com/tommie/v8go errors.cc:54:3: error: no matching function for call to 'free' /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/malloc/_malloc.h:56:7: note: candidate function not viable: no known conversion from 'const char *' to 'void *' for 1st argument

So it's not just my editor (although it's not perfect either - if you have good LSP settings, I'm all ears)

Oh, it's probably because of the const. I guess we should change err to not be const. If we're always allowed to free it, it shouldn't be const. Leave the void* cast for now.

Technically, I think this should use const_cast in C++.

tommie · 2025-05-21T07:42:54Z

value.cc

+  RtnString res = {};
+  res.length = val->Utf8LengthV2(iso);
+  res.data = static_cast<char*>(malloc(res.length));
+  val->WriteUtf8V2(iso, (char*)res.data, res.length);


I suggest sticking to either C- or C++-style casts in one function. :)

tommie · 2025-05-21T07:46:05Z

value.cc

@@ -11,6 +14,29 @@

 using namespace v8;

+RtnString StringToRtnString(v8::Isolate* iso, Local<String> val) {
+  RtnString res = {};


I don't think the {} initialization does anything (the original code had {0} with no purpose there).

RtnString and RtnError should have constructors (or default initializers) to ensure values are cleared.

{} zeros the fields. But I don't know the difference between that and {0}.

However, these two types RtnString and RtnError are exposed to Go code, i.e. Go code actually looks up the field - so I don't think you can add constructors? Because then it would be a C++ type, not a C type?

You could make them "anonymous" (don't know the correct term), like m_ctx where Go code just has pointer values it can pass back to C functions - and then add functions to look up values, but that also seems somewhat cumbersome.

As long as you don't add a vtable, it's just compile-time syntactic sugar for running a function every time a value is created.

Relying on a destructor might be dangerous, since the cgo code wouldn't run it.

I'm not sure I what you mean. But this works - but it generates warnings.

struct RtnError { const char* msg; const char* location; const char* stack; #ifdef __cplusplus RtnError() : msg(0), location(0), stack(0) {}; #endif };

To get "methods" on the type, I changed from typedef struct { ... } RtnError to struct RtnError { }.

This caused compilation errors, both in .h and .go files.

In .h files, extern RtnValue SomeFunction(...); had to be changed to extern struct RtnValue SomeFunction(...)

In .go files, C.RtnValue had to be changed to C.struct_RtnValue.

I could avoid those changes by adding typedef struct RtnError RtnError; though I probably shouldn't use the same identifier?

The warnings are:

./context.h:80:17: warning: 'RunScript' has C-linkage specified, but returns user-defined type 'RtnValue' which is incompatible with C [-Wreturn-type-c-linkage]

But if I remove the initializer from some function creating a RtnError, all tests are still passing, and if I then remove the constructor, it fails at runtime.

But I had the impression from your comments that there is a simpler way?

Oh, I see, this needs to compile in C mode. Of course it does. My bad.

Ok, helper function it is...

Edit The helper function was about another comment... (Does it show there's too much going on for me to keep track. ;)

But here: still need to add back the zero into the intializer to make it do something. https://stackoverflow.com/a/10828333

stroiman force-pushed the value-typeof branch from 502529b to be9fb11 Compare May 20, 2025 10:21

Implement Value.TypeOf()

b09d1cb

stroiman force-pushed the value-typeof branch from be9fb11 to b09d1cb Compare May 20, 2025 10:30

tommie changed the title ~~Implement Value.ToString()~~ Implement Value.TypeOf() May 21, 2025

tommie requested changes May 21, 2025

View reviewed changes

stroiman added 2 commits May 21, 2025 12:50

Remove unnecessary null check

c420dad

Use consistent C-style

0ee6942

Uh oh!

Implement Value.TypeOf() #104

Are you sure you want to change the base?

Implement Value.TypeOf() #104

Uh oh!

Conversation

stroiman commented May 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stroiman commented May 10, 2025

Uh oh!

stroiman commented May 10, 2025

Uh oh!

tommie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tommie May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stroiman May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tommie May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stroiman commented May 10, 2025 •

edited

Loading

tommie May 21, 2025 •

edited

Loading

stroiman May 21, 2025 •

edited

Loading

tommie May 22, 2025 •

edited

Loading