Cleanup of function passed to 'memoize'

[dpino]

Originally this patch was an attempt to fix #225, but it happened to be unrelated (see #225 for details). The patch is kept as a cleanup.

[kbara]

LGTM.

[dpino]

Travis failed. If I run "make && make check" in my laptop everything is fine. According to the log, the failing command is:

pflua-quickcheck --seed=657855364 --iterations=954 properties/opt_eq_unopt data/wingolog.pcap test-filters

If I run it locally I got:

luajit: ../src/pf/quickcheck.lua:51: module 'properties/opt_eq_unopt' not found:
    no field package.preload['properties/opt_eq_unopt']
    no file './properties/opt_eq_unopt.lua'
    no file '/home/dpino/workspace/torch/install/share/luajit-2.1.0-alpha/properties/opt_eq_unopt.lua'
    no file '/usr/local/share/lua/5.1/properties/opt_eq_unopt.lua'
    no file '/usr/local/share/lua/5.1/properties/opt_eq_unopt/init.lua'
    no file '/home/dpino/workspace/torch/install/share/lua/5.1/properties/opt_eq_unopt.lua'
    no file '/home/dpino/workspace/torch/install/share/lua/5.1/properties/opt_eq_unopt/init.lua'
    no file '../src/properties/opt_eq_unopt.lua'
    no file './properties/opt_eq_unopt.so'
    no file '/usr/local/lib/lua/5.1/properties/opt_eq_unopt.so'
    no file '/home/dpino/workspace/torch/install/lib/lua/5.1/properties/opt_eq_unopt.so'
    no file '/usr/local/lib/lua/5.1/loadall.so'
stack traceback:
    [C]: in function 'require'
    ../src/pf/quickcheck.lua:51: in function 'initialize'
    ../src/pf/quickcheck.lua:80: in function 'initialize_from_command_line'
    ./pflua-quickcheck:9: in function 'main'
    ./pflua-quickcheck:13: in main chunk
    [C]: at 0x004061b0

Both in this branch and master. @kbara Any hint?

[kbara]

There are two different issues here; the first should indeed block merging for now. a) The travis failure. That property is indeed failing more often on the changed branch; I'll create a bug report for it. The failure in question (which isn't the only one I've been able to trigger) was:

% ../env pflua-quickcheck --seed=657855364 --iterations=954 properties/opt_eq_unopt data/wingolog.pcap test-filters
Git revision 29493e065fad41634f9a20ed1ce42f6f8ac61d40
The property was falsified.
--- Expanded:
{ "if",
  { "!=",
    "len",
    0 },
  { "<",
    "len",
    { "uint32",
      { "/",
        0,
        "len" } } },
  { "fail" } }
--- Optimized:
{ "!=",
  "len",
  0 }
On packet 21: unoptimized was false, optimized was true

b) The error you've had; have you put a .lua at the end of the property name, making it match the filename? That causes a similar error. Here's the invocation I'm using, along with the results on master; the invocation is the same on your branch, though the results aren't:

tests% ../env pflua-quickcheck  --seed=657855364 --iterations=954 properties/opt_eq_unopt data/wingolog.pcap test-filters
954 iterations succeeded.

[kbara]

Why do you think this fixes issue 255? Were you able to reproduce it? If so, how? https://github.com/SnabbCo/snabbswitch/pull/514 mentions that it's not reproducible on Luke's development machine, and thus far, I haven't been able to reproduce the problem in https://gist.github.com/SnabbBot/8fbed2912cfa49228602#file-log-L490-L514 either.

[kbara]

I've reproduced it, but this fix simply changes the position of the error; we still have a table within a table.

[kbara]

Bug 225 is resolved (Mike's disabled the LuaJIT optimization which caused the problem). I like this patch as an unrelated code cleanup, though. WDYT about amending the commit name and message to reflect that it's a cleanup rather than a fix, then merging it?

[kbara]

Sorry to bikeshed, but can you ditch 'Originally this patch was an attempt to fix #225, but it happened to be unrelated (see #225 for details).' from the patch description? Other than that, LGTM.

[dpino]

The comment is not part of the patch description, it's just a comment in the bugtracker. The patch description is "Cleanup of function passed to 'memoize'". I can still remove it though, should I?

[kbara]

Naw, leave it. I've pulled and the commit looks fine now. The branch name is a bit unfortunate, but I'm ok with it being merged as-is if you're happy to merge it. :)

[dpino]

Yep, that's right, the branch name is now unaccurate. I'm afraid renaming it might be a hassle. I'd merge it as-is.

[kbara]

Agreed; please merge it.