search

aphyr / clj-antlr

Clojure bindings for the ANTLR 4 parser
166 stars 32 forks source link

Update antlr to 4.7.2, switching from deprecated AntlrInputStream #10

Closed willcohen closed 5 years ago

willcohen commented 5 years ago

AntlrInputStream has been deprecated in favor of CharStream in antlr 4.7, which also changes the process needed to get case insensitivity.

While the library's overall functionality switches to use the new char-streams in this PR, for now this leaves antlr-input-stream, input-stream, and case-sensitive-input-stream in place in clj-antlr.common with docstring notes added about deprecation, and leaves CaseInsensitiveInputStream.java present as well, though java compilation gets a deprecation warning on that file.

The antlr javadocs do note the following: "WARNING: If you use both the deprecated and the new streams, you will see a nontrivial performance degradation." For this reason, it might be worth considering removing the functions mentioned above so that users don't end up using them unnecessarily.

aphyr commented 5 years ago

Would you like me to cut a release with these changes now, or batch up some more work first?

willcohen commented 5 years ago

Excellent! I don't think there's anything else to do now that the dependencies are up to date, so I say release whenever you're ready. (And thanks for both the useful library and the quick turnaround!)

aphyr commented 5 years ago

OK. We do have a note about /home/aphyr/attic/clj-antlr/src/java/CaseInsensitiveInputStream.java uses or overrides a deprecated API.--is that something you'd like to resolve?

willcohen commented 5 years ago

Two choices to deal with that warning -- 1) It seems like CaseInsensitiveInputStream.java just inherently relies on deprecated behavior, and anyone using AntlrInputStreams will be getting that deprecation starting with 4.7. CaseChangingCharStream.java uses the new interface. If the old antlr-input-stream, input-stream, and case-sensitive-input-stream functions stay in the common ns, I don't know how to get around that warning and that old class. I left that stuff in to minimize potential breakage. 2) Nothing in the primary functionality of this library, though, actually NEEDS those functions or that java class anymore. As far as I'm concerned, dropping that java file and those associated now-deprecated common functions is a cleaner move. This, though, would be a breaking change for anyone who explicitly is calling on those internal helper functions. As long as users are just using the library the regular way and not dropping down into the functions in common, the new char-streams process with its new case insensitive class will be a drop-in replacement. If they get removed, it might need to go in a note for release notes and be signified with a bump to 0.3, say. If those functions get removed, I think the multi-hinted-let macro could be removed as well. Since the new interface has three different methods for each of the stream types, I couldn't figure out a way to rely on hinting to choose between them without an explicit conditional statement.

aphyr commented 5 years ago

Yeah. It's honestly been forever since I've been in this code, and I'm not maintaining anything that uses it any more, so this is 100% up to you (and anyone else who's a current user!). I'm just here to be a good steward!

How about... let's leave the deprecation notice in for now, so users can at least get access to the new features if they need them, and follow that up with a release to remove deprecated/unused stuff? If you'd like to be especially polite about it, we can add some deprecation logging to calls of those functions in this release, so folks have a chance to be politely warned of the change in behavior.