Skip to content

kernel: unify 'and' for filters in interpreter and executor #2755

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.

Sign up for GitHub

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

Closed
fingolfin wants to merge 2 commits into from
Closed

kernel: unify 'and' for filters in interpreter and executor #2755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e6772e
kernel: add Warning() helper
fingolfin Sep 12, 2018
54c34b7
kernel: unify 'and' for filters in interpreter and executor
fingolfin Sep 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/error.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,19 @@ void CheckSameLength(const Char * desc, const Char *leftName, const Char *rightN
}
}

/****************************************************************************
**
*F Warning
*/
void Warning(const Char * msg, Int arg1, Int arg2)
{
Pr("Warning, ", 0, 0);
Pr(msg, arg1, arg2);
if (strcmp("*stdin*", GetInputFilename()) != 0)
Pr(" in %s:%d", (Int)GetInputFilename(), GetInputLineNumber());
Pr("\n", 0, 0);
}

/****************************************************************************
**
*V GVarFuncs . . . . . . . . . . . . . . . . . . list of functions to export
Expand Down
7 changes: 7 additions & 0 deletions src/error.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,13 @@ void CheckIsDenseList(const Char * desc, const Char * listName, Obj list);
void CheckSameLength(const Char * desc, const Char *leftName, const Char *rightName, Obj left, Obj right);


/****************************************************************************
**
*F Warning
*/
extern void Warning(const Char * msg, Int arg1, Int arg2);


/****************************************************************************
**
*F * * * * * * * * * * * * * * initialize module * * * * * * * * * * * * * *
Expand Down
20 changes: 16 additions & 4 deletions src/exprs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,23 +183,35 @@ Obj EvalAnd (
}

/* handle the 'and' of two filters */
else if ( TNUM_OBJ(opL) == T_FUNCTION ) {
else if (IS_PSEUDO_FILTER(opL)) {
if (!IS_FILTER(opL)) {
// support this for backwards compatibility; see discussion on
// https://github.com/gap-system/gap/pull/2732
Warning("operation '%g' used in AND-filter is not a filter",
(Int)NAME_FUNC(opL), 0);
}
tmp = READ_EXPR(expr, 1);
opR = EVAL_EXPR( tmp );
if ( TNUM_OBJ(opR) == T_FUNCTION ) {
if (IS_PSEUDO_FILTER(opR)) {
if (!IS_FILTER(opR)) {
// support this for backwards compatibility; see discussion on
// https://github.com/gap-system/gap/pull/2732
Warning("operation '%g' used in AND-filter is not a filter",
(Int)NAME_FUNC(opR), 0);
}
return NewAndFilter( opL, opR );
}
else {
ErrorQuit(
"<expr> must be 'true' or 'false' (not a %s)",
"<expr> must be a filter (not a %s)",
(Int)TNAM_OBJ(opL), 0L );
}
}

/* signal an error */
else {
ErrorQuit(
"<expr> must be 'true' or 'false' (not a %s)",
"<expr> must be 'true' or 'false' or a filter (not a %s)",
(Int)TNAM_OBJ(opL), 0L );
}

Expand Down
20 changes: 16 additions & 4 deletions src/intrprtr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1341,21 +1341,33 @@ void IntrAnd ( void )
}

/* handle the 'and' of two filters */
else if ( IS_OPERATION(opL) ) {
if ( IS_OPERATION(opR) ) {
else if (IS_PSEUDO_FILTER(opL)) {
if (!IS_FILTER(opL)) {
// support this for backwards compatibility; see discussion on
// https://github.com/gap-system/gap/pull/2732
Warning("operation '%g' used in AND-filter is not a filter\n",
(Int)NAME_FUNC(opL), 0);
}
if (IS_PSEUDO_FILTER(opR)) {
if (!IS_FILTER(opR)) {
// support this for backwards compatibility; see discussion on
// https://github.com/gap-system/gap/pull/2732
Warning("operation '%g' used in AND-filter is not a filter",
(Int)NAME_FUNC(opR), 0);
}
PushObj( NewAndFilter( opL, opR ) );
}
else {
ErrorQuit(
"<expr> must be 'true' or 'false' (not a %s)",
"<expr> must be a filter (not a %s)",
(Int)TNAM_OBJ(opL), 0L );
}
}

/* signal an error */
else {
ErrorQuit(
"<expr> must be 'true' or 'false' (not a %s)",
"<expr> must be 'true' or 'false' or a filter (not a %s)",
(Int)TNAM_OBJ(opL), 0L );
}
}
Expand Down
10 changes: 10 additions & 0 deletions src/opers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,16 @@ static inline Int IS_FILTER(Obj oper)
return v & OPER_IS_FILTER;
}

// temporary HACK, until all affected packages are fixed
static inline Int IS_PSEUDO_FILTER(Obj oper)
{
if (!IS_OPERATION(oper))
return 0;
Obj flags = FLAGS_FILT(oper);
return flags && TNUM_OBJ(flags) == T_FLAGS;
}


/****************************************************************************
**
*F SET_IS_FILTER( <oper> ) . . . . . . . . . . . mark operation as a filter
Expand Down
69 changes: 63 additions & 6 deletions tst/testinstall/boolean.tst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,17 @@ gap> ViewString(true); ViewString(false); ViewString(fail);
gap> TNAM_OBJ(fail);
"boolean or fail"

# test error handling
# crazy stuff that is accepted by interpreter and executor
gap> false and 1;
false
gap> true or 1;
true
gap> function() return false and 1; end();
false
gap> function() return true or 1; end();
true

# test error handling in interpreter
gap> not 1;
Error, <expr> must be 'true' or 'false' (not a integer)
gap> false or 1;
Expand All @@ -64,17 +74,64 @@ Error, <expr> must be 'true' or 'false' (not a integer)
gap> true and 1;
Error, <expr> must be 'true' or 'false' (not a integer)
gap> 1 and true;
Error, <expr> must be 'true' or 'false' (not a integer)
Error, <expr> must be 'true' or 'false' or a filter (not a integer)
gap> ReturnTrue and ReturnTrue;
Error, <expr> must be 'true' or 'false' (not a function)
Error, <expr> must be 'true' or 'false' or a filter (not a function)
gap> ReturnTrue and true;
Error, <expr> must be 'true' or 'false' (not a function)
Error, <expr> must be 'true' or 'false' or a filter (not a function)
gap> IsAssociative and ReturnTrue;
Error, <expr> must be 'true' or 'false' (not a function)
Error, <expr> must be a filter (not a function)
gap> IsAssociative and true;
Error, <expr> must be 'true' or 'false' (not a function)
Error, <expr> must be a filter (not a function)
gap> IsAssociative and Center;
Warning, operation 'Centre' used in AND-filter is not a filter in stream:1
<Filter "(IsAssociative and Centre)">
gap> IsAssociative and FirstOp;
Error, <expr> must be a filter (not a function)
gap> true and IsAssociative;
Error, <expr> must be 'true' or 'false' (not a function)
gap> Center and IsAssociative;
Warning, operation 'Centre' used in AND-filter is not a filter
in stream:1
<Filter "(Centre and IsAssociative)">
gap> FirstOp and IsAssociative;
Error, <expr> must be 'true' or 'false' or a filter (not a function)
gap> IsAssociative and IsAssociative;
<Property "IsAssociative">

# test error handling in executor
gap> function() return not 1; end();
Error, <expr> must be 'true' or 'false' (not a integer)
gap> function() return false or 1; end();
Error, <expr> must be 'true' or 'false' (not a integer)
gap> function() return 1 or false; end();
Error, <expr> must be 'true' or 'false' (not a integer)
gap> function() return true and 1; end();
Error, <expr> must be 'true' or 'false' (not a integer)
gap> function() return 1 and true; end();
Error, <expr> must be 'true' or 'false' or a filter (not a integer)
gap> function() return ReturnTrue and ReturnTrue; end();
Error, <expr> must be 'true' or 'false' or a filter (not a function)
gap> function() return ReturnTrue and true; end();
Error, <expr> must be 'true' or 'false' or a filter (not a function)
gap> function() return IsAssociative and ReturnTrue; end();
Error, <expr> must be a filter (not a function)
gap> function() return IsAssociative and true; end();
Error, <expr> must be a filter (not a function)
gap> function() return IsAssociative and Center; end();
Warning, operation 'Centre' used in AND-filter is not a filter in stream:1
<Filter "(IsAssociative and Centre)">
gap> function() return IsAssociative and FirstOp; end();
Error, <expr> must be a filter (not a function)
gap> function() return true and IsAssociative; end();
Error, <expr> must be 'true' or 'false' (not a function)
gap> function() return Center and IsAssociative; end();
Warning, operation 'Centre' used in AND-filter is not a filter in stream:1
<Filter "(Centre and IsAssociative)">
Copy link
Member Author

@fingolfin fingolfin Aug 28, 2018

Choose a reason for hiding this comment

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

Here the warning can be seen in effect

gap> function() return FirstOp and IsAssociative; end();
Error, <expr> must be 'true' or 'false' or a filter (not a function)
gap> function() return IsAssociative and IsAssociative; end();
<Property "IsAssociative">

#
gap> STOP_TEST( "boolean.tst", 1);
Expand Down