Split c files

[stroiman]

This is not a complete job yet, but paves a road forward.

There is some naming inconsistencies, that I've not done anything about. Example

Here the IsolatePtr references a v8 class

typedef v8::Isolate Isolate;
typedef Isolate IsolatePtr

Here the ContextPtr references a wrapper type in the v8go C++ code.

typedef struct m_ctx m_ctx;
typedef m_ctx ContextPtr

And then there, a v8go C++ struct ScriptCompilerCachedData but a ScriptCompilerCachedDataPtr pointing to a v8 type.

typedef v8::ScriptCompiler::CachedData* ScriptCompilerCachedDataPtr;

typedef struct {
  ScriptCompilerCachedDataPtr ptr;
  const uint8_t* data;
  int length;
  int rejected;
} ScriptCompilerCachedData;

I don't think the Ptr is really useful here though. It seems that the XyzPtr types are needed for Go code; it wouldn't compile when I just created a *v8Isolate in Go alone - but I could have made a mistake.

At any rate I would consider adopting a style like this:

#idfef __cplusplus
typedef v8::Isolate v8Isolate;
#else
typedef struct v8Isolate v8Isolate
#endif
typedef Isolate v8IsolatePtr

As now it's clear what is a v8 type, and what is v8go type.

Avoid using namespace

The header file has a using namespace v8. That is generally a not-so-good practice. Including a header file shouldn't cause namespaces to be included.

Only include what's needed

The v8go.cc and .h file included the entire v8.h header file. This just includes what's necessary for the particular files; speeding up compilation

Use forward declarations

A type definition of structs/classes doesn't need to know the full definition of used types, just the size in memory. For classes and structs, I've created forward declarations to avoid having to include unnecessary header files.

Some non-trivial have been placed in forward-declarations, mostly as the C++ classes are non-trivial in their forward declarations.

I think there might be tools that can automate header file dependency code, i.e., remove unnecessary includes, add necessary ones, create forward decl. But I have no experience using any of those, so I don't know how they would be set up, and what the are capable and incapable of.

[stroiman]

And added a clang config file so the editor should pick the current style on foatting

[tommie]

Thanks for taking the time to contribute! Some small thoughts on code structure.

[tommie]

Consider moving this to errors.h. It only exists to add an error to a value, so I think it belongs in the realm of error handling (and this file is small, with a vague scope.)

[stroiman]

Yeah, I had trouble placing this. I didn't think it so much of as an error, but more like converting the result to idiomatic go.

But I wouldn't mind putting it in errors. I didn't look too much at what the "errors.h" really contains, as extracting the errors file was mostly the result of, if I move A, I also need to move B.

[tommie]

My problem is this file is too small and unfocused: no one will think of looking at this file to find RtnValue. A file that is about mapping error handling domains makes more sense to me. That said, it's not a strong opinion.

[stroiman]

I've moved it to errors.h for now. Maybe a sensible place will reveal itself later :)

I don't have a strong opinion either.

[stroiman]

Btw, on "small files", there is also utils.h/utils.cc containing string copying functionality ...

Opinion on those?

[tommie]

I think they're fine. Having one slush bucket is better than two, and probably unavoidable. :)

[tommie]

Should we remove the almost empty file?

[stroiman]

Damn, I thought I did that.

[stroiman]

Ahh, just realised I've made quite the mistake.

I have this checked out under go/src/github/stroiman/v8go, but when I run the test suite, the tests are exercising code under go/src/github/tommie/... 🙈

Code probably works as intended, as I've used it from my own project; which uses the "rewrite" feature of go mod to point to my own version.

[stroiman]

I made the original comment:

I don't think the Ptr is really useful here though. It seems that the XyzPtr types are needed for Go code; it wouldn't compile when I just created a *v8Isolate in Go alone - but I could have made a mistake.

But it appears that I did make a mistake, if I forward declare a v8Isolate for instance

#ifdef __cplusplus
namespace v8 { class Isolate } // ...
typedef v8::Isolate v8Isolate
#else
typedef struct v8Isolate v8Isolate
#endif

I can declare/define the functions as

extern v8Isolate* NewIsolate();
// ...
v8Isolate* NewIsolate() { };

Then the Go code is happy with

type Isolate struct {
	ptr *C.v8Isolate
}
func NewIsolate() *Isolate {
	initializeIfNecessary()
	iso := &Isolate{
		ptr: C.NewIsolate(),
		cbs: make(map[int]FunctionCallbackWithError),
	}
}

So I would be in favour of just removing the XyzPtr typedefs.

Wouldn't do it in this PR, just cleanup when working on new functionality.

[tommie]

go vet complains of stdint.h (and stddef.h for size_t) missing in value.h.

[stroiman]

Ah thanks, just added them, was heading out the door, so didn't see what errors, were produced. Maybe I'll check up on it later, or tomorrow.

[tommie]

Tests are happy now. This looks great!

Just the question about removing interop.h and the reference to it.

[tommie]

@stroiman pushed 0 commits.

Thank you, GitHub email notification, for letting me know nothing happened. :)