sanitise global data allocation; fix a void return #8

dimpase · 2017-10-12T09:47:34Z

Global data should be allocated in *.c
files, and declared (with extern qualifier) in *.h
Otherwise it is asking for trouble (surely it does not work on OSX).

As well, a non-void function cannot have "return;" - fixed that too.

Global data should be allocated in *.c files, and declared (with extern qualifier) in *.h Otherwise it is asking for trouble (surely it does not work on OSX). As well, a non-void function cannot have "return;" - fixed that too.

dimpase · 2017-10-12T09:48:18Z

This is a part of the effort to port to OSX, see https://trac.sagemath.org/ticket/24015

embray · 2017-10-12T10:16:36Z

@dimpase Thanks--this is exactly what I was going to do. I'm not sure how best to deal with fmemopen. It's probably not strictly necessary (it's also not too hard to implement a drop-in replacement but not trivial either).

dimpase · 2017-10-12T11:11:24Z

I've replaced fmemopen with fopen. There is also an open_memstream to deal with.

dimpase · 2017-10-12T11:40:54Z

open_memstream is harder to fix, as one has to understand the global scheme of things under the GC used here. Should the string obtained from the stream after it is closed be GC-collectable?

miguelmarco · 2017-10-12T11:43:10Z

Is this branch tested and ready for merge?

dimpase · 2017-10-12T13:58:27Z

This pull requests appears to work on Linux.

miguelmarco · 2017-10-12T14:07:15Z

I have also run some tests on linux, and nothing seems to be broken. Is this enough to solve the OSX issue or should I wait to make a new release?

dimpase · 2017-10-12T15:07:29Z

For OSX we need to provide a replacement for open_memstream, or else refactor the code using it (it is just one function, p_show). Could you give an upper bound on the size of its output, so that we can just allocate the buffer once and do sprintf to this buffer instead of fprintf to the (unsupported) stream? On 12 Oct 2017 15:07, "miguelmarco" <notifications@github.com> wrote: I have also run some tests on linux, and nothing seems to be broken. Is this enough to solve the OSX issue or should I wait to make a new release? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABN8HELRFZzcw07D94_uiqtzmOFMoXp8ks5srh0TgaJpZM4P2vm6> .

miguelmarco · 2017-10-12T15:24:46Z

Could you give an upper bound on the size of its output, so that we can
just allocate the buffer once and do sprintf to this buffer instead of
fprintf to the (unsupported) stream?

I can't think of a bound that is not overkill on most cases.

What would be the canonical way to append to a string on OSX?

miguelmarco · 2017-10-12T16:50:37Z

One possible solution would be to just remove the p_show function. In fact, the Sage interface doesn't even use it (it uses the homfly_polynomial_dict function which directly extracts the information from the Poly struct).

However, the tests should be adapted in that case.

to do this, we replace fscanfs with sscanfs--- the disadvantage is that we have to manually advance the place to start to read the string after each scanf; fortunately we have %n format parameter to do this easily.

dimpase · 2017-10-12T21:14:31Z

OK, the latest commit at least makes k_read portable. Let me see about p_show...

replacing fprintf with the sprintf, and manual advancing of the string position to write to. here the annoying problem is that we don't have an automatically growing buffer, so we just allocate 100K---for the time being

dimpase · 2017-10-12T23:01:10Z

to make this work on OSX, a newer than the current (7.2f) version of Boehm GC is needed;
with 7.2f the test of libhomfly fails, with the master from https://github.com/ivmai/bdwgc/ it does work---at least standalone (it also works within Sage).

now the string to hold the output is grown dynamically, with a safety region.

dimpase · 2017-10-13T13:30:21Z

OK, ready, tested on Linux and OSX.

miguelmarco · 2017-10-13T15:23:55Z

Reviewing now.

miguelmarco · 2017-10-13T15:32:13Z

lib/knot.c

    return FALSE;
  }
-  fscanf(f, "%d ", &links);
+  sscanf(f, "%d%n", &links, &pos);
+  f += pos;
  for (i=0; i<links; ++i)                    /* how many pieces of string */


This kind of updating the position of the f pointer could have side effects. Wouldn't it be safer to use a copy instead of the pointer passed to the function?

This is a call by value. Nothing will happen to f outside the scope of k_read(). (to change f inside k_read(), you'd need to pass &f, i.e. its address).

miguelmarco · 2017-10-13T15:34:53Z

lib/poly.c



-  stream = open_memstream(&bp, &size);
+  stream = (char *)GC_MALLOC(sizeof(char) * 100000);
+  bp = stream;
  if (!p->len)


How did you decide the 100000 size limit?

miguelmarco · 2017-10-13T16:00:40Z

Besides my minor concern about the side effects of moving the f pointer (which, according to the tests, don't even seem to have any effect), looks good.

dimpase

This is a C call by value, no need to worry about side effects.

dimpase · 2017-10-13T16:32:08Z

lib/knot.c

    return FALSE;
  }
-  fscanf(f, "%d ", &links);
+  sscanf(f, "%d%n", &links, &pos);
+  f += pos;
  for (i=0; i<links; ++i)                    /* how many pieces of string */


This is a call by value. Nothing will happen to f outside the scope of k_read(). (to change f inside k_read(), you'd need to pass &f, i.e. its address).

miguelmarco · 2017-10-13T22:46:49Z

Ok. Let me know when it is finally ready, and I will merge it and do a new release.

dimpase · 2017-10-14T10:36:41Z

it is done in the sense of C code. You can also add the travis stuff. it reports error on OSX as GC 7.6.0 that comes semi-standard is not new enough.

miguelmarco · 2017-10-14T10:39:14Z

IS the travis stuff needed for building on OSX?

miguelmarco · 2017-10-14T10:57:31Z

Merged. The tarball is available here

dimpase · 2017-10-15T09:18:46Z

Could you also do a release on https://github.com/miguelmarco/libhomfly/releases ?

dimpase · 2017-10-15T09:22:47Z

As you might have already found out, .travis.yml is a configuration script for Travis CI, a continuous integration (i.e. testing) service available for free for github (and not only) repos.
You can sign up for it and do automatic testing of each new commit; you can see
the example output of the repo fork on my account here.

E.g. you can test on OSX even if you don't have OSX on any machine...

miguelmarco · 2017-10-15T13:44:53Z

I created the release; but beware that, since the files generated by autoconf are not tracked by git, the tarball in the github release needs to go through
autoreconf --install before running the usual configure && make flow.

sanitise global data allocation; fix a void return

bc86017

Global data should be allocated in *.c files, and declared (with extern qualifier) in *.h Otherwise it is asking for trouble (surely it does not work on OSX). As well, a non-void function cannot have "return;" - fixed that too.

certainly, not NULL, but the resulting "0" string

b752cb2

removed fmemopen

11c3e9a

to do this, we replace fscanfs with sscanfs--- the disadvantage is that we have to manually advance the place to start to read the string after each scanf; fortunately we have %n format parameter to do this easily.

removed open_memstream (POC - needs work)

a5657c8

replacing fprintf with the sprintf, and manual advancing of the string position to write to. here the annoying problem is that we don't have an automatically growing buffer, so we just allocate 100K---for the time being

refactored p_show's string buffer management, added safety

4034b8f

now the string to hold the output is grown dynamically, with a safety region.

miguelmarco reviewed Oct 13, 2017

View reviewed changes

dimpase commented Oct 13, 2017

View reviewed changes

dimpase added 5 commits October 13, 2017 22:00

Create .travis.yml

07d1f9a

Update .travis.yml

c21b5a1

trying to start travis-ci

8201aa6

add OSX testing

9a33f8f

adjusting travis config

a80861b

miguelmarco merged commit c5a3fa2 into miguelmarco:master Oct 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sanitise global data allocation; fix a void return #8

sanitise global data allocation; fix a void return #8

dimpase commented Oct 12, 2017

dimpase commented Oct 12, 2017

embray commented Oct 12, 2017

dimpase commented Oct 12, 2017

dimpase commented Oct 12, 2017

miguelmarco commented Oct 12, 2017

dimpase commented Oct 12, 2017

miguelmarco commented Oct 12, 2017

dimpase commented Oct 12, 2017 via email

miguelmarco commented Oct 12, 2017

miguelmarco commented Oct 12, 2017 •

edited

dimpase commented Oct 12, 2017

dimpase commented Oct 12, 2017 •

edited

dimpase commented Oct 13, 2017

miguelmarco commented Oct 13, 2017

miguelmarco Oct 13, 2017

dimpase Oct 13, 2017

miguelmarco Oct 13, 2017

miguelmarco commented Oct 13, 2017

dimpase left a comment

dimpase Oct 13, 2017

miguelmarco commented Oct 13, 2017

dimpase commented Oct 14, 2017

miguelmarco commented Oct 14, 2017

miguelmarco commented Oct 14, 2017 •

edited

dimpase commented Oct 15, 2017

dimpase commented Oct 15, 2017

miguelmarco commented Oct 15, 2017

sanitise global data allocation; fix a void return #8

sanitise global data allocation; fix a void return #8

Conversation

dimpase commented Oct 12, 2017

dimpase commented Oct 12, 2017

embray commented Oct 12, 2017

dimpase commented Oct 12, 2017

dimpase commented Oct 12, 2017

miguelmarco commented Oct 12, 2017

dimpase commented Oct 12, 2017

miguelmarco commented Oct 12, 2017

dimpase commented Oct 12, 2017 via email

miguelmarco commented Oct 12, 2017

miguelmarco commented Oct 12, 2017 • edited

dimpase commented Oct 12, 2017

dimpase commented Oct 12, 2017 • edited

dimpase commented Oct 13, 2017

miguelmarco commented Oct 13, 2017

miguelmarco Oct 13, 2017

Choose a reason for hiding this comment

dimpase Oct 13, 2017

Choose a reason for hiding this comment

miguelmarco Oct 13, 2017

Choose a reason for hiding this comment

miguelmarco commented Oct 13, 2017

dimpase left a comment

Choose a reason for hiding this comment

dimpase Oct 13, 2017

Choose a reason for hiding this comment

miguelmarco commented Oct 13, 2017

dimpase commented Oct 14, 2017

miguelmarco commented Oct 14, 2017

miguelmarco commented Oct 14, 2017 • edited

dimpase commented Oct 15, 2017

dimpase commented Oct 15, 2017

miguelmarco commented Oct 15, 2017

miguelmarco commented Oct 12, 2017 •

edited

dimpase commented Oct 12, 2017 •

edited

miguelmarco commented Oct 14, 2017 •

edited