search

lutaml / expressir

Ruby parser for the ISO EXPRESS language
3 stars 3 forks source link

Code review #113

Closed maxirmx closed 2 years ago

maxirmx commented 2 years ago

  1. CI inconsistency. Test and release scripts uses very diffrent toolchains: ri tests run in native ubuntu/macos/mingw environment while release versions are built using rake-compiler-dock cross-compile toolchains. If low level issues like #74 are considered such an approach is very risky since successful tests results are not applicable to release versions. IMHO it shall be changed to use consistent toolchain(s) across all builds.

  2. CI inconsistency. It looks like rake script was inherited from nokogiri but in fact it does not use nokogiti verification facilities as applies to OS compatibility and correct linkage.

  3. Hard-to-maintain implementation of core express_parser.cpp file. It is very unusual to see cpp file which has 19K LoC with 1K LoC functions and copy-paste implementation of 200 similar classes. A randomly emerging issue issue like #74 (again :) ) creates a problem that cannot be managed since the only method to identify it is code review. Probably some templates and some metaprogramming can radically reduce code complexity

  4. There is no instrumentation in the pipeline, not even static code analizer for Ruby code

ronaldtse commented 2 years ago

IMHO it shall be changed to use consistent toolchain(s) across all builds.

Agere.

express_parser.cpp

Isn't this file generated by https://github.com/camertron/antlr4-native-rb/ ?

There is no instrumentation in the pipeline, not even static code analizer for Ruby code

Can you help us set that up? Thanks!

maxirmx commented 2 years ago

express_parser.cpp

Isn't this file generated by https://github.com/camertron/antlr4-native-rb/ ?

It is. If we take it as given then something has to be done with code generator at antlr3-native-rb https://github.com/camertron/antlr4-native-rb#caveats says: Contexts (i.e. the ctx variables in the examples above) and other objects that are created by ANTLR's C++ runtime are automatically cleaned up without the Ruby interpreter's knowledge.

This sounds really strange to me me.