[WIP] add length of included_files array

[drewwells]

This extends the API to add the length of the included_files array. Without this, it is difficult to determine the length of the C array exposed via the API interface.

[coveralls]

Coverage decreased (-2.41%) to 77.99% when pulling 060c95a on drewwells:feature/importslength into 8e7a294 on sass:master.

[coveralls]

Coverage decreased (-0.01%) to 80.39% when pulling 060c95a on drewwells:feature/importslength into 8e7a294 on sass:master.

[mgreter]

IMO it's actually not very hard 😉

char** incs = sass_get_included_files;
while (*incs) { process(*incs); ++incs; } 

[drewwells]

for perl :p. cgo isn't so gentle!
https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices

On Sat, Mar 28, 2015 at 8:08 PM Marcel Greter notifications@github.com
wrote:

IMO it's actually not very hard [image: 😉]

char** incs = sass_get_included_files;while (_incs) { process(_incs); ++incs; }

—
Reply to this email directly or view it on GitHub
#1001 (comment).

[drewwells]

I thought about doing:

for i := range sass_get_included_files {
  if sass_get_included_files[i] == nil {
    length = i
  }
}

but this might cause bad runtime exceptions

[coveralls]

Coverage decreased (-0.01%) to 80.39% when pulling 304a8e2 on drewwells:feature/importslength into 8e7a294 on sass:master.

[mgreter]

If you need to compute the length beforehand you could use plain C:

size_t l = 0; while (*incs) { ++l; ++incs; } 

[drewwells]

True!, but this makes it available in the API which felt like a good service to others coming across the same issue. Also, num_included_files was already in the interface but not used anywhere: https://github.com/sass/libsass/blob/master/sass_interface.h#L58

[mgreter]

Sass_Interface is obsolete and will be removed soon! I'd rather have a function return the result from the snippet I posted above in the API, than keeping a counter in sync with the null terminated array. I don't see any benefit to have this cached on our side! We would also need to take care of updating it when sass_context_take_included_files is called etc.

[mgreter]

A shorter and future-proof (but slightly slower, if called multiple times) equivalent would be:

ADDAPI size_t ADDCALL sass_context_get_included_files_size (struct Sass_Context* ctx);
size_t ADDCALL sass_context_get_included_files_size (struct Sass_Context* ctx)
{ size_t l = 0; auto i = ctx->included_files; while (i && *i) { ++i; ++l; } return l; }

[mgreter]

Please check #1000 as I have a commit there which includes the code from my latest comment!

[am11]

This might be of your interest: node-sass/src/binding.cpp#L135-L148:

  char** included_files = sass_context_get_included_files(ctx); 

  if (included_files) { 
    for (int i = 0; included_files[i] != nullptr; ++i) { 
      // do something
    }
  }

[QuLogic]

You don't even need the if:

for (; *included_files != nullptr; included_files++) {
 // do something
}

[am11]

@QuLogic, thanks for the suggestion. It is fixed by sass/node-sass@8a194d0. 👍

However, we need the iterator index to insert the values in v8 array.

[QuLogic]

No, wait, actually, there might be a case where the array is NULL, if parsing fails. Might be safer for you to leave the condition in then, if you're still checking it in all cases.

[am11]

Yes we are checking it in all cases, but is there state, when libsass returns NULL for included_files?

I think it always returns the valid pointer, since it converts C++ vector (context.hpp#L43) to array of C strings (sass_context.cpp#L475), while allocating memory in all cases: sass_interface.cpp#L35.

Should we consider it safe?

[mgreter]

Libsass doesn't guarantee it (ie. if you call it too eraly, before compilation has taken place). I would say play safe here, as it doesn't really imrpove the performance anyway!

[drewwells]

Closing in favor of #1000. Thanks @mgreter