C++ runtime cleanup

[Jan Mikkelsen]

Hi,

I have been using the C++ runtime and have been updating some of the code to fix problems flagged by our build system.

• Interface changes in ParseTreePatternMatcher and XPath to avoid object slicing. Mike Lischke mentioned on github that some code hasn’t really seen use — This might help it get closer.
• ATNDeserializationOptions, ParseTreeMatch, ParseTreePattern, Vocabulary, ParseInfo: Remove virtual functions and make final. These are used by value and so derived classes don’t seem to be useful.
• Copy constructor and assignment operator deletion to avoid accidental object slicing elsewhere.

Also in a current pull request is a change to how IntervalSet works and some concurrency bugfixes.

In case anyone is interested, I have in a pending branch with the changes:

https://github.com/jm-mikkelsen/antlr4/tree/pending

If you see something you don’t like in the changed, please let me know.

Regards,

Jan Mikkelsen

[Ivan Kochurkin]

I think GitHub issues is a more appropriate place for such suggestions.

[Jim Idle]

I believe that this is the correct place for this. I also feel that these sound like good changes, but Mike has to review. 

When people post anything in github, they quite often get shot down in flames, so I think Jan is doing the right thing here. He can later make a pull request with the same details. He's just asking if this is wanted. 

I dont think you were being rude btw Ivan, I just dont want to put people off :)

Jim

--
You received this message because you are subscribed to the Google Groups "antlr-discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to antlr-discussi...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Mike Lischke]

Well, I've been thinking about similar optimizations and even applied a few (e.g. see the ParseTree hierarchy simplifications). However, the big question is how close we want to stay to the original Java code. Keep in mind it's still under development and once there are runtime changes all target authors have to convert them to their target language. If the differences have grown too big this might produce quite some problems, especially when some time went by since you last looked at that.

In that sense things that might be used to extend functionality (like virtual methods) need a second look to decide what will likely not need to stay as is (especially since Java tends to use virtual members a lot and be it only for academic reasons). Other stuff, like language specific changes are often uncritical. Measures to avoid object slicing are certainly welcome. Also simplifications around profiling/statistics are very likely no problem. However, I can't say much about the pattern stuff. For me this is rather an exotic feature, there seem to be no unit tests and it's quite a lot of code for a simple thing (that XPath simulation). You should file a PR so we can see what you changed.

Mike

--

http://www.soft-gems.net

[Eric Vergnaud]

All,

just to clarify, GitHub has far less readers and contributors than this discussion group, hence why I often redirect people here.

We unfortunately see many "bug reports" which are really support requests. These can be more rapidly handled here, and very importantly the discussion benefits to a much wider community.

As per genuine improvement proposals, they can be discussed here or via GitHub, and it is indeed necessary to discuss them beforehand rather than submit a PR.

Antlr is a 25 years investment from @parr. A number of 'great ideas' have already been considered carefully in the past.

[Jan Mikkelsen]

Thanks Eric and Jim.

I did want to expose the changes to discussion. The first PR I submitted on GitHub was large and involved a bunch of discussion (thanks Mike!) but I suspected that there were more mailing list readers than people looking at pull requests.

On the 25 year investment: Yes, I understand and appreciate that. I think I even contributed some small patches to PCCTS in the ‘90s.

Regards,

Jan.

[Jan Mikkelsen]

I understand the requirement to stay close to the Java code.

In the C++ runtime one of the basic decisions is which classes to pass by value and which to pass by reference. Java pass by reference looks just like a C++ pass by value, so it’s easy to do the wrong thing or miss a case when translating code. I think it’s important to make those decisions at the class level (eg. declare the class final without virtual methods, or removing the copy constructor and operator=()) so that use of the class is enforced one way or another at compile time.

Once you do pass by value, you can get rid of the virtual methods and should prevent inheritance to avoid object slicing. This makes a lot of sense for runtime-generated objects. My changes to Interval and IntervalSet fall into that category.

Where you do want inheritance, passing by value is a bug — That’s what I changed in the XPath and ParseTreePatternMatcher code. At this point I’m not planning to be a user of that code either, but the code as it stands slices objects.

I also removed “read only” attributes: The natural way to do that in C++ is through constness, that avoids needing to maintain user code to check a read-only flag.

Most (all?) of the changes in this patch address those issues. 

On a pull request: I was waiting for feedback on the one I have currently open before opening a new one. Do you want me to add these changes to the one currently open, or wait for it to be resolved one way or another and then open a new one?

Jan.