Add object get property names

[GustavoCaso]

Related to #213

I'm trying to give a try to my first PR that adds new logic both in Go and in C++.

I'm not 100% sure that want I'm doing is the right thing to do and I would love some guidance if possible. @dylanahsmith @rogchap

Basically, I'm trying to return an array with all the property names from V8 using the obj->GetPropertyNames(local_ctx).

Is there anything that I'm missing?

Thanks 😄

[codecov]

Codecov Report

Merging #260 (fab0a50) into master (5e91d3d) will decrease coverage by 0.25%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #260      +/-   ##
==========================================
- Coverage   95.86%   95.60%   -0.26%     
==========================================
  Files          17       17              
  Lines         580      592      +12     
==========================================
+ Hits          556      566      +10     
- Misses         15       16       +1     
- Partials        9       10       +1     

Impacted Files	Coverage Δ
context.go	97.14% <80.00%> (-2.86%)	⬇️
object.go	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e91d3d...fab0a50. Read the comment docs.

[genevieve]

Will this miss freeing strings[0] if the index doesn't get there? Should the middle param be i >=0

[dylanahsmith]

Good catch!

We can't use i >=0 though, since the integer is unsigned, so that would always be true. If we wanted to do that, we could change i to int64_t.

Would the code maybe be clearer if it matched the outer loop, e.g.

      length = i;
      for (i = 0; i < length; i++) {
        free(&strings[i]);

or by using a separate loop variable?

[genevieve]

I think this inner loop is supposed to be from 0 to i whatever i is because it might need to free all strings if an error occurs which means that it might not be equal to length and we'd be trying to free elements we don't have yet.

[genevieve]

Maybe it should be:

  for (uint32_t i = 0; i < length; i++) {
    Local<Value> name_from_array;

    if (!names->Get(local_ctx, i).ToLocal(&name_from_array)) {
      for (j = 0; j < i; j++) {
        free(&strings[j]);
      }
      free(strings);
      rtn.error = ExceptionError(try_catch, iso, local_ctx);
      return rtn;
    }
    String::Utf8Value ds(iso, name_from_array);
    strings[i] = CopyString(ds);
  }

[dylanahsmith]

I think this inner loop is supposed to be from 0 to i

right, which is my code snippet started with length = i;

Your suggestion is what I meant by "or by using a separate loop variable", except that the for loop line needs to declare the j variable by prefixing its assignment with its type: for (uint32_t j = 0; j < i; j++) {

[genevieve]

Ah right let’s go with your suggestion. Think I got confused by the inner loop variable having the same name.

[genevieve]

properties*

[genevieve]

Since we know the size of result, we should be able to preallocate:

	result := make([]string, len(slice))
	for i := 0; i < len(slice); i++ {
		s := slice[i]
		defer C.free(unsafe.Pointer(s))
		result[i] = C.GoString(s)
	}
	defer C.free(unsafe.Pointer(rtn.strings))
	return result, nil