constant for empty prefix set

[Ondrej Zajicek]

On Tue, Feb 22, 2022 at 08:18:35PM +0100, Alexander Zubkov wrote:
> Adding new pathches (v2 - full patch, v1-v2 - incremental from previous):
> 
> 1) Modified as_path_filter function - added additional parameter to
> choose between using set or key parameter. Now empty set does not work
> like a zero integer constant. Also thought about passing one struct
> f_val parameter instead of those 3 parameters, but that requires
> adding include of filter/data.h to nest/attrs.h, and that looks like
> causing a chicken and an egg problem.

OK. Also you could pass f_val *, so you would avoid the include.
Although 3 parameters are also fine.


> 2) Added the case of empty T_SET to be promoted to T_PREFIX_SET in
> f_const_promotion function. There is need to create an empty trie, I
> used "f_new_trie(cfg_mem, 0)", but not sure cfg_mem is good to use
> here. It works with simple tests.

One solution would be a global constant empty trie, so you can just set
pointer to that. Alternative would be just have NULL like in T_SET,
but that would mean to add several special cases to trie functions,
so that is probably worse approach. The difference is that for that
trees NULLs are naturally valid values, while tries have header.


> By the way, not completely sure that
> during promotion, the value that is overwrited, is a copy of the
> constant value (because it can be a global named constant) and not the
> original value itself. I made some tests and looks like that after
> assignment, the global constan value remains to be a T_SET, I hope
> this is not because of some optimization.

Yes it is a copy stored in FI_CONSTANT instruction:

decl.m4:488  struct f_val *c = &arg->i_FI_CONSTANT.val

It is copied during parsing from the global symbol:

config.Y:731  $$ = f_new_inst(FI_CONSTANT, *($1->val))


> 3) Made filter/delete to act the same on T_PATH. For some reason
> delete(path, integer) was allowed, but filter(path, integer) wasn't.

Nak. That is intentional and documented. And the same behavior is for
T_*LIST. Both filter(path, integer) and filter(clist, pair) is pretty
useless and would require extending existing *list-handling code just
to do that.


> There are actually still problems with types:
> 
> 4) I found out that passing arguments to the function does not check
> actual types during runtime. So we can actually pass any type to a
> function and it will even work, given that operators applied to the
> variables are supported.

That is oversight on my side, there should be static typechecks that also
do promotions. Will look at that.


> 5) When prefix set variable is assigned an empty set, it is promoted
> to the T_PREFIX_SET, of course, but after that it becomes not equal to
> "[]", because the type is different. But that can be considered a
> feature, not a bug.

Yes, if FI_EQ used ARG_SAME_TYPE(), the typecheck would force promotion,
but our equality check accepts any pair of objects (and returns false
for object of different types instead of failing).

I will look into this by implementing some restricted variant of
ARG_SAME_TYPE() that forces promotion but otherwise ignores mismatched
types.

(although one could argue that if we already know in parse time that
comparison would fail due to different types, then something is wrong.)


> 6) Because of (4), as function arguments are not checked and are not
> promoted, if we have an argument of type prefix set, and pass [] to
> that function, it will remain of type T_SET. And if we try to assign
> it to another prefix set variable, there will be a runtime error like
> this:
> 
> bird: filters, line 12: Argument 1 of VAR_SET must be of type set, got type set
> 
> This "feature" affects quads also.


-- 
Elen sila lumenn' omentielvo

Ondrej 'Santiago' Zajicek (email: santiago at crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."