constant for empty prefix set

[Alexander Zubkov]

Updated patch. Again full and incremental.

On Wed, Feb 23, 2022 at 4:48 AM Ondrej Zajicek <santiago at crfreenet.org> wrote:
>
> 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.

Yes, I chose an additional parameter because with a pointer it will be
impossible to call the function with this parameter "inline".

>
>
> > 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.

OK, created a global constant. There is a pointer to the memory pool
in the f_trie structure, but I believe it is not used in this case. It
may be even good to have it set to NULL for this constant in case
somebody tries to add elements to that trie.

>
> > 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))
>

Thanks, I looked at these too, but still had some doubts. :)

>
> > 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.
>

Heh, did not check that in the documentation. :) OK, returned the
original behaviour. But on the other hand, although it is useless, I
see no problems with allowing it just for the symmetry.

>
> > 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.

The other possibility is to alter val_same()/val_compare() functions
to handle that situation like for quads. It just the matter of what
behaviour is more preferrable. Current patch has this variant
implemented taking into account that all empty prefix sets use the
same emtpy trie constant.
But altering val_compare() will make the comaprison of typed variables
with empty sets to be true, for example prefix set with int set. On
the other hand, currently it does the same with different sets
represented as T_SET. And as I understand, it is also possible to
compare an ip set with a quad set.

>
> (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."
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bird-empty-set.patch-v2-v3
Type: application/octet-stream
Size: 2773 bytes
Desc: not available
URL: <http://trubka.network.cz/pipermail/bird-users/attachments/20220223/6b78f585/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bird-empty-set.patch-v3
Type: application/octet-stream
Size: 16521 bytes
Desc: not available
URL: <http://trubka.network.cz/pipermail/bird-users/attachments/20220223/6b78f585/attachment-0001.obj>