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. 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. 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. 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. 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. 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. 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. On Tue, Feb 22, 2022 at 5:18 AM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Tue, Feb 22, 2022 at 03:02:55AM +0100, Alexander Zubkov wrote:
Also to your previous question:
I also think now - why at all do we need a typed empty set? I think it is possible to add an untyped empty set, that will act as a "joker" with any types. It should fit better to the current syntax now without introducing a lot of new constructions.
It breaks static type controls or at least make them more complicated. Note that our types for sets are already pretty broken - we have just T_SET and T_PREFIX_SET in implementation, although different types of sets are used in filter language (e.g. 'int set' in variable definition) and matching types are tested just in runtime (based on set->from.type). But let's assume we would fix this, we would have types T_INT_SET, I_IP_SET, ..., T_PREFIX_SET for each set type.
Then defining untyped empty set means it would be a new type (T_EMPTY_SET) different from others, it would require some exceptions in type checks (e.g. FI_VAR_SET just checks that type of value is the same as type of variable) and many expressions that accepts or returns value of just one type now would accepts or returns values of two different types. We would have to extend type checking system for this or implement some concept of subtyping or type coercion.
Sure, I had the same concerns about this idea with T_EMPTY_SET. But on the other hand, with an empty T_SET we still need to make an exception at least for prefixes. And if, as you said, there are more types of sets in the future, than there will be still more exceptions. So in some circumstances, I think, both ways will have around the same number of exceptions. But currently, the empty T_SET looks easier of course.
So, are you against an untyped empty set constant given the mentioned problems? Or you are unhappy about the complexity of implementing and maintaining that idea, but anyway consider it acceptable?
I checked how IP/QUAD situation is solved and it is cleaner than i thought. There is already constant promotion function f_const_promotion() that can be extended to handle empty sets. So seems to me that with the current state of BIRD code this is simplest approach:
There should be two empty sets values, one for T_SET (which behaves like in your patch) and one for T_PREFIX_SET (which would be just empty trie). Constant [] would generate the T_SET empty constant, but f_const_promotion() would handle promotion of T_SET empty constant to T_PREFIX_SET empty constant when needed. That should fix issues with variables/arguments.
-- Elen sila lumenn' omentielvo
Ondrej 'Santiago' Zajicek (email: santiago@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."