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

Different results in direct and in functions when using ~ #2469

Closed
ChrisJefferson opened this issue May 16, 2018 · 4 comments
Closed

Different results in direct and in functions when using ~ #2469

ChrisJefferson opened this issue May 16, 2018 · 4 comments

Comments

@ChrisJefferson
Copy link
Contributor

ChrisJefferson commented May 16, 2018

Consider this:

gap> [Length(~),Length(~),Length(~)];
[ 0, 1, 2 ]
gap> ({} ->  [Length(~),Length(~),Length(~)])();
[ 3, 3, 3 ]

I'd like us to standardise on one of these. I have a preference for the first one, because it allows fixing a seperate problem (the partially built lists when x is in a function clearly have their length set to past their last element when partially built, which can upset various bits of code in GAP).

@ChrisJefferson ChrisJefferson changed the title Different results in direct and in functions Different results in direct and in functions when using ~ May 16, 2018
@fingolfin
Copy link
Member

(I took the liberty of adjusting your example to directly show both cases)

Personally, I am not worried about this fringe case; ~ is not really well-defined or documented, other than "read the code". If somebody uses code like the above, they deserve to suffer ;-).

Thing is, your proposed solution amounts to a minor de-optimization of the executor. Now, while I hope it won't matter in practice even for artificial examples, it still is a de-optimization. That makes me really wonder if "fixing it" this way is a good idea? I mean, we could just as well simply document this diverging behaviour...

Anyway, I am not that concerned, so, whatever... ;-)

@ChrisJefferson
Copy link
Contributor Author

I have a slightly bigger concern that it might be possible, with sufficently crazy abuse of ~, to get crashes (but I haven't found one yet) -- many C functions in GAP do assume that ELM_LIST(list, LEN_LIST(list)); is bound (if LEN_LIST(list) is > 0 of course). Of course, there is an argument people shouldn't be passing ~ to arbitary functions anyway!

@ChrisJefferson
Copy link
Contributor Author

Update: I can make GAP crash by doing sufficently horrible things with ~, like the following. Actually, this wouldn't be that hard to fix, and we already have a special case for code with ~, so making that even slower wouldn't upset me that much.

So, looking at the code, my suggested fix is:

  1. Fix this problem, and in general make ~ more resiliant (which basically means in the function which fills a list which contains ~, be more careful). This will only slow down lists which include a use of ~.

  2. Still disable the check on non-dense plists, as there will still be partially built non-dense plists without a final element internally (lists which do not contain a ~), but no way for a user to see them, or for them ever to be passed to another function.

gap> g := function(l) Remove(l); return 1; end;                       
function( l ) ... end                                                 
gap> f := function() x := [1,2,3,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1
,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,4,5,6,7,8,g(~),12,13,14,1,1,1,1,1
,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,15,16,17,
18,19,110]; end;                                                      
function(  ) ... end                                                  
gap> for i in [1..1000] do f(); od;                                   
gap> GASMAN("collect");                                               
Panic: Gasman found a bogus header

@fingolfin
Copy link
Member

I believe this was fixed in PR #2477 (it'd be really good to reference issues from PRs and vice versa)

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

No branches or pull requests

2 participants