Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Purge loaded module #57

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Purge loaded module #57

wants to merge 2 commits into from

Conversation

ten0s
Copy link

@ten0s ten0s commented Jan 7, 2017

I was trying to work through this Quickcheck video https://vimeo.com/104007760 and
immediately found that Nifty doesn't reload the module itself after recompile. This commit
fixes this. Please notice that the code doesn't load the module itself.

This will purge already loaded code and will make new code
available automatically
@kostis
Copy link
Contributor

kostis commented Jan 17, 2017

We are trying to understand what this pull request does and the need for it. Why would nifty:compile/1, which does not load any code, need to purge the old one as a side-effect? Can you please provide an example (perhaps in the form of a test case)?

Also, in the code itself please add erlang: before the call to check_old_code so that it is clear this is not a module-local function.

@ten0s
Copy link
Author

ten0s commented Jan 18, 2017

Even though nitfy:compile doesn't not load any code, it sets up a path to newly
generated code and in the tutorial Nitfy is used interactively.

@ten0s
Copy link
Author

ten0s commented Jan 18, 2017

Testing a simple C program

https://vimeo.com/104007760

$ git clone https://github.com/rjmh/q
$ cd q/
$ wget https://raw.githubusercontent.com/voila/pbt/master/QuickCheck/q_eqc.erl
$ erl
> nifty:compile("q.c", q).
generating...
==> q (compile)
src/q_remote.erl:59: function new/1 already defined
src/q_remote.erl:3: Warning: function new/1 already exported
Compiling src/q_remote.erl failed:
ERROR: compile failed while processing /home/ten0s/tmp/test2/q/q: rebar_abort
{error,compile}

PROBLEM 1:

'new' already defined compile error. I think it's a problem with Nifty too. Generated code should not create a function with such a ubiquitous name like 'new'.

FIX:

rename 'new' to 'queue' in q.c

> nifty:compile("q.c", q).
Compiling c_src/q_nif.c
c_src/q_nif.c:4347:28: warning: implicit declaration of function 'ptr_to_record_117' is invalid in C99 [-Wimplicit-function-declaration]
erlarg___wait_terminated = ptr_to_record_117(env, (ptr_t)(&(cunion->__wait_terminated)));
....
2 warnings and 13 errors generated.
ERROR: compile failed while processing /home/ten0s/tmp/test2/q/q: rebar_abort
{error,compile}

PROBLEM 2:

Not sure what it is, but probably Nifty can't work without a header file.

FIX:

create q.h, move the queue struct to q.h and include q.h in q.c

$ cat > q.h << EOF
typedef struct queue
{ int inp;
  int outp;
  int size;
  int *buf;
} Queue;

Queue *queue(int n);
int put(Queue *q, int n);
int get(Queue *q);
int size(Queue *q);
EOF
> nifty:compile("q.h", q, nifty_utils:add_sources(["q.c"], [])).
generating...
==> q (compile)
Compiled src/q_remote.erl
Compiled src/q.erl
Compiling c_src/q_nif.c
Compiling /home/ten0s/tmp/test2/q/q.c
ok

> rr("q/include/q.hrl").
['struct queue']
> Q = q:queue(10).
{140524887486336,"q.Queue *"}
> nifty:dereference(Q).
#'struct queue'{inp = 0,outp = 0,size = 10,
                buf = {140524887486288,"q.int *"}}
> q:put(Q, 12).
12
> q:get(Q).
12

OK. It works now! Let's proceed with the video.

PROBLEM 3:

Port q_eqc to work with Proper

FIX:

  • remove eqc*.hrl
  • add -include_lib("proper/include/proper.hrl").
  • replace {call,,new,} with {call,,queue,}
  • add proper_fsm: to commands/1, run_commands/1 state_names/1
> c(q_eqc).
q_eqc.erl:53: Warning: variable 'Res' is unused
q_eqc.erl:53: Warning: variable 'V' is unused
{ok,q_eqc}

> proper:quickcheck(q_eqc:prop_q()).
proper:quickcheck(q_eqc:prop_q()).
............................!
Failed: After 29 test(s).
...

PROBLEM 4:

Fix q.c to pass tests (from the video)

FIX:

  • in Queue * queue(int): replace {0,0,n,buff} with {0,0,n+1,buff}
  • in int size(Queue *): replace (q->inp - q->outp) with (q->inp - q->outp + q->size)
> nifty:compile("q.h", q, nifty_utils:add_sources(["q.c"], [])).
...
ok
> proper:quickcheck(q_eqc:prop_q()).
proper:quickcheck(q_eqc:prop_q()).
............................!
Failed: After 46 test(s).
...

PROBLEM 5:

After fixing C code nifty:compile doesn't reload the code.

FIX:

Purge old code

> code:purge(q), code:delete(q), code:purge(q).
false

proper:quickcheck(q_eqc:prop_q()).
....................................................................................................
OK: Passed 100 test(s).

38% {created,{q,size,1}}
32% {created,{q,put,2}}
20% {created,{q,get,1}}
9% {init_state,{q,queue,1}}
true

The bottom line is Nifty currently doesn't support interactive work like eqc_c does.

@kostis
Copy link
Contributor

kostis commented Jan 18, 2017

You are of course right that nifty:compile/3 sets up the path, thus preparing for an upcoming load. Perhaps this is inconsistent. But to us, it does not make sense to purge the code without doing a load. So, either nifty:compile/3 should purge the code and load the new one, or the purge needs to happen in a separate nifty:load function that should be introduced. What would you prefer?

Thanks for the detailed example, but parts of it are different issues and do not belong in this pull request. Please open other issues to discuss them.

@ten0s
Copy link
Author

ten0s commented Jan 18, 2017

I agree that compile doesn't sound like load/purge. But from the user's point of view I saw the issue and proposed my solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants