On Thu, Jun 15, 2023 at 03:57:10AM +0200, Alexander Zubkov wrote:
Hi,
While waiting for the fate of the previous patches, I was thinking about that thing about using keywords as symbols. So here is another longread. :)
Now it is not possible to mix a keyword and a keyword as a symbol together. Here is what I mean. With current master bird if I use config: ... protocol device {} function role() { int role = 1; return role; } template bgp { local role peer; }
Hi You are right, but note that keywords collide just with top-level symbols (e.g. protocols, tables, functions, ...), while scoped symbols do not affect it, e.g, this is okay: function xyz() { int role = 1; return role; } template bgp { local role peer; } While it is inconvenient, it preserves backward compatibility (compared to the previous approach, where keywords always dominate symbols), as a configuration valid for an older version of BIRD is valid in a newer version of BIRD that adds new keywords (the configuration may contain such names used as symbols, but not as keywords).
Because "role" is converted to a symbol and it is not possible to use it as a keyword anymore. I thought if something can be done about that and I probably have a solution. See the attached patch. It maybe not the best design, just to show the idea. I change the order of detecting symbol and keyword in the lexer, so it always returns a keyword for a keyword, and it is converted into a symbol only in the parser when it is used as a symbol. As we can hit such symbol keyword many times, I check if it has already been defined and create a new one if needed. Then I replace mentions of CF_SYM_KNOWN with symbol_known, which can be CF_SYM_KNOWN or kw_sym. So, with this patch applied, both configs are successfully parsed and work well.
We thought about this approach, but it has issues with conflicts in our rather irregular grammar. Note that current kw_sym is almost empty, i originally planned it to contain all keywords (unless explicitly excluded), and i would likely to do that soon. Even with almost empty kw_sym, your patch added 2 grammar conflicts. If we add most symbols to kw_sym, it will cause many conflicts. OTOH, the current approach accepts kw_sym only in 'symbol' [*], which is much scarced in grammar, and therefore causes minimum conflicts. [*] Which is in-fact more like symbol_unknown, but also accepts known symbols for purpose of redefining them in different scope. I would prefer to avoid such changes now, as Maria is doing some rework on symbols and we have some divergence in symbol processing between master and 3.0 branches.
Also, I think that the current realization in bird relies on the fact that lexer would not have symbols parsed in advance, i.e. that further mentions of the keyword should return CF_SYM_KNOWN. But if it is not the case and the lexer parses forward for some reason (before the parser creates the symbol for the keyword) it would not return CF_SYM_KNOWN. I don't know the internals of the lexer and parser much, maybe it is impossible situation (parser does not backtrack, etc.). And there is no need to worry here.
Yes, it is kind of strange, in long-term, it would make more sense to move all symbol processing from lexer to parser.
Some additional notes.
My previous remark regarding the missing "|" in "kw_sym: MIN MAX" seems to be correct, because current bird master fails on such config:
Yes that is true. I just added it ad-hoc in order to have some kw_sym defined in nest/config.Y, so it is not empty when BIRD is compiled without BGP.
But if I try to call "eval role()" from cli, I get an error: "runtime error", and the daemon logs this:
bird: filters, line 2: Argument 1 of VAR_INIT must be of type int, got type void
So it looks like it tries to initialize it with the void value and fails to do it. I haven't tracked the source of this bug.
Will look at this. -- 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."