This project has moved and is read-only. For the latest updates, please go here.

Irony source code review: TermFlags review

Jan 31, 2015 at 12:06 AM
Hi Roman,

I've been going through the Irony source code to try and iron out some issues I'm getting in the way my grammar is being handled. Specifically, I'm looking at the TermFlags enumeration. The way it is implemented right now, each of the flags can be turned on independently from the other flags. But a lot of those combinations do not make sense. An obvious example is setting the IsOperator and IsList combination for the same terminal.

So what I'm thinking is that there are some sets of flags which are mutually exclusive and a terminal should belong to one and only one of these. Hence those flags could be moved to a separate enum and add a property to the BnfTerm which allows one of these options to be set.

Truly independent flags could also be moved to boolean properties on the BnfTerm. My example here is the IsNotReported flag, this seems like it could be set or not set independently of whatever other flags are set with no logical impact.

What I would like is your input on the categorisation of these different flags. Here is the way I see it:

// Mutually exclusive flags:
IsPunctuation // Can the meaning of this be replaced by the following? TermFlags.Outline, TermFlags.NoAstNode
IsDelimiter // This doesn't really seem to be used anywhere, what is its purpose?
IsNonGrammar // this is effectively a comment isn't it? How else is it used?

// Is this relevant ONLY when the IsOperator flag is set? If it can be used in other ways then it is probably independent.

// Do these options all relate to one another? Maybe the Ast flags can be moved to a separate flags enumeration?

// Independent flags:

// Not used anywhere that I can see.
Feb 3, 2015 at 12:16 AM
thanks for the suggestion. Honestly, I'm not a fan of making lot of bool flags, as soon as I see 2 or more bool properties, I look at possibility of converting them to Flags enum. First, less code, typing and less maintenance. Memory also might be an issue, although less so. The main reason - changing anything, adding or removing flags does not change the shape of the object, just the field list in enum. Mutual compatibility or exclusiveness - this is a dangerous path, you never know what might be needed in particular case, so free-form flagset is more flexible option. yes, it opens the way for mistakes, but definitely the dev must know what he's doing when he uses the flag and tries to set it. Flagsets instead of bool props are used all over the place in .NET core libraries, I'm not alone in this preference.
Dev friendliness - this is a questionable intent. For the first time user Irony seems too easy on the surface, samples work great, but as soon as you try to create a simple grammar of your own, you get a cold shower of conflicts, and no idea what to do. It turns out, it's not that simple, you have to get into LALR parsing theory and pay your dues. So Irony is not a microwave that must be safe to use by any old granma or a child, so interfaces must be tweaked in every way to be friendly - the cost of learning is unavoidable. And if you hit the wrong button - you won't burn your house, just get a few errors. Time to read and learn.
Finally, the current version is so outdated (in regards to modern challenges to parser construction), that it needs a complete rewrite. When it happens - another question. Ask people in Redmond why .NET still has no solid parsing technology in the core framework, and all hangs on dudes like me - who has to pay the bills and do other things to earn a paycheck...
thanks again
Feb 3, 2015 at 2:49 AM
Hi Roman,

Firstly I just want to say I'm deeply respectful for you in what you've done by providing the Irony framework as an open source tool. Not only that but the level of support that you give to it as evidenced by your responsiveness in these forums is beyond the call of duty. So if this comes across as critical, its because I value this project enough to see it improved.

I reckon that a list of boolean properties and a list of flag enums are an equivalent ways to express the same intention. And I'm not against flag enums in an absolute sense. So I'm accepting of your comments regarding the benefits of them and I'm in no way saying that you should get rid of them because bool properties are always better. I would include these additional comments from a philisophical point of view:

The flags should all have the same concerns. I see in the enum above, flags relating to a BnfTerm's use in the grammar, flags relating to what produced the term, flags relating to AST generation and flags relating to how it should be displayed in an editor. My opinion would be to put related flags into separate enums.

You may argue that this is just code bloat but there are tangible benefits to be had. For instance, if you looked at a set of flags and decided that they were truly mutually exclusive, then you could change it to a non-flag enum without affecting the other flags.

Regarding the public interface, some of the flags may not need to be part of the public interface. For example IsNonScanner should only be set by Irony and if its value is required by external code its accessor can be made public.

Also a lot of these flags are set through helper methods, rather than directly e.g. MarkReservedWords. If reserved words were marked using this method only it would make it possible to completely change the way reserved words are managed. For instance, they could be identified by their presence in a list leaving the BnfTerm without any knowledge of what a reserved word is (this would only work if BnfTerm never needed to know whether it was a reserved word or not).

Your comments about not knowing what might be needed in a particular case, I would say "you aren't gonna need it". Enums that are targeted would be a flexible way to include additional functionality. Note that a lot of combinations of flags are incompatible with Irony's implementation. As an example, grammars are validated to ensure that no terminal has the IsTransient and IsList flag set simultaneously but presumably there are many other combinations which are not checked.

Flags require more dedication from the developer as well as it is easy to perform the wrong bitwise operation on them or assign a flags value when you actually want to combine it with those that are already set. Tough luck might be the response under these circumstances. But I think you have to admit that having a non-flag enum makes it difficult to make a lot of these mistakes and bool properties basically eliminate them entirely.

I'd just like to finish by saying, thanks (in case this comes across as less than appreciative). I'm definitely in the same boat needing to pay bills. I wanted to contribute to the project but if you aren't in a position to discuss it, I understand.
Feb 5, 2015 at 7:35 AM
Edited Feb 5, 2015 at 8:05 AM
thanks! and let's just agree to disagree on one agreed point - these are not major issues.
Overall, you're taking a bit wrong approach to Irony as it is now. It is NOT an RTM release from BigoHard team of hundred devs, testers and PMs, all brushed up, cleaned up of accidental garbage, etc etc. It had always been work in progress, I was just pushing later and later versions, and as a work in progress, it is very difficult to maintain clean and nice interfaces and APIs. Like you suggest to split flags into several enums separating flag semantics. Trust me, when you change and change things, move things around, trying to maintain this kind of quality is nearly impossible. I admit - there are many places where I do not conform to high standards - things that should not be changed by client code should be immutable (readonly collections etc), things should be hidden and exposed carefully etc. No such luck. So I was doing it for a while, and then the process stopped. What you see is a snapshot at this moment
For now, Irony is at rest, sorry, busy with other staff...
So it goes :)