Skip to content

Conversation

@ChrisJefferson
Copy link
Contributor

Changes the notation for variadic argument lists from (a,b,arg) (where arg is a special name) to (a,b, ...p) (where p can be any identifier, including arg). This doesn't change functionality at all, it just means that we don't break any code which was using arg as an argument name.

This patch leaves the older (new) notation in for now, if we like this I'll remove it and fix the standard library.

For the curious, after all this stuff is finished, all GAP 4.7 code will behave identically, this will only add new notation.

@markuspf
Copy link
Member

markuspf commented Dec 9, 2015

Just for the record, two slightly alternative syntax variants were discussed, namely using

f := function(a,b,args...) ... end;

or

f := function(a,b,args[]) ... end;

All variants are similarly easy to implement. My (not very strong) order of preference is args[] > args... > ...args.

Also notable is the fact that we now support different names for the variadic argument list, i.e.

f := function(x1,x2,..xs)

end;

works, and xs is the list of parameters passed after the second one.

@stevelinton
Copy link
Contributor

I would favour the x[] syntax as being more obviously related to concepts already in GAP, rather than concepts from Python or C++.

@olexandr-konovalov
Copy link
Member

My order of preferences would be:

  • f(a, b, x[])
  • f(a, b, ...x)
  • f(a, b, x...)

I like f(a, b, x[]) the most because it conveys the fact that x will be a list whenever I will see x in the function body much better than the other two.

My second choice wins over my third choice because f(a, b, ...x) separates variadic argument x from other arguments more visibly than f(a, b, x...). And speculating further, accidental cutting of x in ...x will result in syntactically invalid code, while accidental cutting of dots in x... will produce syntactically correct but wrong code.

The last one has also a nice point though: the brain may expand f(a, b, x...) into f(a, b, x1, x2, x3, ...) easier than f(a, b, ...x). But since my preference is the 1st one, I will stop here.

@rbehrends
Copy link
Contributor

For what it's worth (i.e. my two Eurocents, give or take :) ), I favor the x... option, because it seems to most closely approximate conventional mathematical notation; the ...x notation feels like it's the wrong way around. I'm not particularly enthusiastic about the x[] notation (which makes it look like an expression, not a declaration).

@fingolfin
Copy link
Member

My two cents: I favor x... over ...x for the reasons @rbehrends gave; and I dislike x[] -- but any of them is vastly better than breaking compatibility with existing code.

@markuspf
Copy link
Member

As I mentioned before, I will not stand in the way of any choice that is made.

@fingolfin
Copy link
Member

@alex-konovalov wrote:

I like f(a, b, x[]) the most because it conveys the fact that x will be a list whenever I will see x in the function body much better than the other two.

That is actually one of the reasons why I dislike it: It gives the incorrect impression that this is about marking an argument as receiving a list; but this is just a byproduct, after all, other arguments may also receive lists. The true intent here is to mark the argument as one receiving extra variadic arguments...

In particular, I am concerned that it might mislead people with experiences in other languages which have similar x[] syntax. So somebody with a background in C/C++ might think they have to use [] to mark arguments that expect a list. They would also expect it to "work" for arbitrary arguments, and would not guess that it actually is used for variadic arguments. The ... OTOH is familar to those people. So for these people, the x[] syntax might be a bad choice.

Now, there are of course lots of other people who are not familiar with either notation, and thus would not find either more or less confusing. I believe most users would in fact be rather neutral about the choice of syntax. So, I have a group of users for which x[] might end up negative, and a group where it is neutral. Hence in total, it seems inferior to me compared to x... or ...x.. :-)

I also still don't get why you are concerned about people "cutting" their code accidentally. If that happens, you are always screwed. Moreover, I actually don't understand your specific example: if I botch a copy & paste of f(a, b, x...) the way you describe it, I'd end up with ``f(a, b, xand that actually is also syntactically invalid. Of course a novice programmer might end up "fixing" that by adding a)`. But are you then also concerned about a novice programmer copy & pasting

if x then
   funcA();
   funcB();
fi;

as

if x then
   funcA();

and then adding fi;, accidentally changing the meaning of the code?

@olexandr-konovalov
Copy link
Member

Well, perhaps if someone uses coding style

something:=function(
                                   a, b, c...
                                 )

and cuts dots, then one could still get a valid input. But I've never seen such. I am not fixated on this at all - I think this is something to take into account only if you have two choices which seem absolutely identical otherwise. You can not predict all ways to shoot yourself in a foot in advance. As I said, I will be happy with any of three choices. Seems to me like we are leaning towards ...x at the moment, and Steve has a good point about .. instead of ...

@fingolfin
Copy link
Member

Here is another (weak) reason why I personally prefer ... over []: It fits nicely with another syntax I'd like to see, and mentioned on the mailing list, inspired by Julia: Allow writing

func(1,2,3,list...)

or

func(1,2,3,...list)

as synonym for

CallFuncList(func, Concatenation([1,2,3], list));

which I would find highly useful. Of course we could still do that, even if we do not use ... or .. for variadic arguments. And we could also use a syntax like

func(1,2,3,list[])

but that would then be ambiguous for the parser, and feels contrived.

@markuspf
Copy link
Member

I start leaning towards args... for multiple reasons. I still won't be in any implementation's way though.

@olexandr-konovalov
Copy link
Member

And I start leaning towards *x but also will be happy with anything.

BTW, one should check readability of the code both WITH spaces and WITHOUT spaces. To help you compare all six options suggested so far:

f(a,b,c,*x)
f(a,b,c,x[])
f(a,b,c,x..)
f(a,b,c,..x)
f(a,b,c,x...)
f(a,b,c,...x)

and

f( a, b, c, *x )
f( a, b, c, x[] )
f( a, b, c, x.. )
f( a, b, c, ..x )
f( a, b, c, x... )
f( a, b, c, ...x )

@olexandr-konovalov
Copy link
Member

@fingolfin
Copy link
Member

Looking at the poll so far, it seems that "x..." is the clear "winner" so far. @ChrisJefferson would you mind rewriting this PR to use that syntax?

src/read.c Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indention here is different from the surrounding code (which seems to mix tabs and spaces). I think it is best to match the indenting mode of the surrounding code, to keep the code readable. For now, that is (we can fix the indention globally later on, perhaps during GAP days).

@ChrisJefferson
Copy link
Contributor Author

Hopefully now cleaned up.

In the end, x... was the clear winner, so gone with that.

Removed function(x, arg) style variadic arguments. function(arg) still works. Added tests, and made sure the parser gives sensible messages when users confuse .. and ....

No documentation yet.

@stevelinton
Copy link
Contributor

Did you change the (few) examples of function (x, arg) that I had already put in?

If not I can.

Steve

On 15 Dec 2015, at 12:49, Christopher Jefferson [email protected] wrote:

Hopefully now cleaned up.

In the end, x... was the clear winner, so gone with that.

Removed function(x, arg) style variadic arguments. function(arg) still works. Added tests, and made sure the parser gives sensible messages when users confuse .. and ....

No documentation yet.


Reply to this email directly or view it on GitHub.

@ChrisJefferson
Copy link
Contributor Author

Yup (although I just noticed there was a couple in the docs, so I'm doing that now).

@stevelinton
Copy link
Contributor

One consequence of this that we MUST do a second 4.8 beta

@stevelinton
Copy link
Contributor

Thanks

On 15 Dec 2015, at 12:59, Christopher Jefferson [email protected] wrote:

Yup (although I just noticed there was a couple in the docs, so I'm doing that now).


Reply to this email directly or view it on GitHub.

@ChrisJefferson ChrisJefferson force-pushed the varargs branch 2 times, most recently from 5a7fa84 to 452ccee Compare December 15, 2015 13:26
@markuspf
Copy link
Member

I think if @fingolfin is happy with this, we should go ahead and merge, and get the next beta out so we can do 4.8.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a full stop here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sentence is weird in the revised form... It doesn't really make sense to only talk about the "single argument" case here, right?

@olexandr-konovalov olexandr-konovalov added this to the GAP 4.8.1 milestone Dec 15, 2015
lib/function.g Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out this check can fail: Apparently is possible for nams to be fail, when looking at C level code. The resulting function, when printed, is shown like this:

function ( <<arg-1>> )
    <<kernel or compiled code>>
end

A reproducible (but not yet minimal) test case:

METHODS_OPERATION(Socle,1);

@ChrisJefferson
Copy link
Contributor Author

Now updated, with @fingolfin 's many comments fixed.

The problem of printing functions without argument lists is fixed, but not in the .tst files (as I didn't track down a small function for testing yet).

fingolfin added a commit that referenced this pull request Dec 18, 2015
New variadic arguments implementation
@fingolfin fingolfin merged commit 5755f1c into gap-system:master Dec 18, 2015
@fingolfin
Copy link
Member

Good job, Chris, thanks!

@olexandr-konovalov olexandr-konovalov changed the title [WIP] New variadic arguments implementation New variadic arguments implementation Dec 19, 2015
@olexandr-konovalov
Copy link
Member

The diffs below occur in the test of manual examples - I will fix this:

########> Diff in [ "./../../lib/function.g", 361, 376 ]
# Input is:
PrintNumberFromDigits := function ( arg )
   CallFuncList( Print, arg );
   Print( "\n" );
  end;
# Expected output:
function( arg ) ... end
# But found:
function( arg... ) ... end
########
########> Diff in [ "./../../lib/function.g", 361, 376 ]
# Input is:
PrintDigits := function ( arg )
   Print( arg );
   Print( "\n" );
  end;
# Expected output:
function( arg ) ... end
# But found:
function( arg... ) ... end
########
########> Diff in [ "./../../lib/function.g", 399, 406 ]
# Input is:
f:=ReturnTrue;  
# Expected output:
function( arg ) ... end
# But found:
function( arg... ) ... end
########
########> Diff in [ "./../../lib/function.g", 426, 433 ]
# Input is:
f:=ReturnFalse;  
# Expected output:
function( arg ) ... end
# But found:
function( arg... ) ... end
########
########> Diff in [ "./../../lib/function.g", 453, 460 ]
# Input is:
oops:=ReturnFail;  
# Expected output:
function( arg ) ... end
# But found:
function( arg... ) ... end
########
########> Diff in [ "./../../lib/function.g", 479, 484 ]
# Input is:
n:=ReturnNothing;
# Expected output:
function( object ) ... end
# But found:
function( object... ) ... end
########
########> Diff in [ "./../../lib/function.g", 504, 513 ]
# Input is:
f:=ReturnFirst;
# Expected output:
function( object ) ... end
# But found:
function( object... ) ... end
########
########> Diff in [ "./mloop.xml", 624, 642 ]
# Input is:
ErrorNoTraceBack := function(arg) # arg is special variable that GAP
                                    # knows to treat as list of arg's
    local SavedOnBreak, ENTBOnBreak;
    SavedOnBreak := OnBreak;        # save current value of OnBreak

    ENTBOnBreak := function()       # our `local' OnBreak
    local s;
      for s in arg do
        Print(s);
      od;
      OnBreak := SavedOnBreak;      # restore OnBreak afterwards
    end;

    OnBreak := ENTBOnBreak;
    Error();
  end;
# Expected output:
function( arg ) ... end
# But found:
function( arg... ) ... end
########
########> Diff in [ "./../../lib/teaching.g", 288, 294 ]
# Input is:
isbntest:=CheckDigitTestFunction(10,11,[1,2,3,4,5,6,7,8,9,-1]); 
# Expected output:
function( arg ) ... end
# But found:
function( arg... ) ... end
########

olexandr-konovalov pushed a commit that referenced this pull request Dec 20, 2015
hulpke pushed a commit to hulpke/gap that referenced this pull request Dec 21, 2015
hulpke added a commit to hulpke/gap that referenced this pull request Dec 21, 2015
* commit '2ee33a75ae033755b6fce81551bf7c5ffc1594e1':
  Updated index entries for arg and Argument
  Added /:=, Fix to List(Filtered construct
  Make `Filtered' and `List(Filtered' its own parsed unit
  Parse `Filtered' construct.
  Translate ` as record component. <...> as list. Further function names.
  Declare second argument as IsObject as IsInt is not yet available
  Updated examples changed after PR gap-system#410
  Fixed examples after PR gap-system#391
  improve auto generated manual example test files, add some auto-generated files to .gitignore
  Properly translate `elif' even if parse tree uses `else if'
  Facilty to translate function names to GAP equivalents
  Remove extern/gmp-5.0.5.tar.gz
  Better match list for `?Random`
  Remove arg representing variadic arguments in function lists, except when it is the only argument
  Add ... style varargs with tests and documentation
  Build GMP with C++ support enabled
@ChrisJefferson ChrisJefferson deleted the varargs branch April 15, 2016 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants