Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/generalize parser #80

Closed
wants to merge 3 commits into from

Conversation

geier1993
Copy link
Contributor

This branch is based on the other PR with SFINAE capabilites for eckit::Translator - we may also just merge this one.

Changes in the branch require C++17 - before C++11 dependencies like metkit, fdb etc. have also compiled because compilers just emitted warnings. Hence PR for other repositories have been created as well.

Motivation:

  • Abstract ObjectParser, JSONParser, YAMLParser with a template argument ObjectBuilder that is describing how to create an object. The trait ObjectBuilderTraits is checking for some requirements to describe the builder (will look nicer with concepts)..
  • In Multio we have a new Metadata type that is not using eckit::Value and is more typed. For transport messages are encoded/decoded via JSON and hence having a decoder that avoids using eckit::Value is desired.
  • As the whole "Parser" structure of the Object/JSONparser seems to be shaped by the YAMLParser, I also refactored the YAML parser - that comes with more specific requirements on the YAMLParser.

Changes:

  • Most code has been put in header files with eckit::Value exchanged to specific calls to ObjectBuilder
  • parseString is returning std::string as parsing is all about strings and sometimes string values need to be accessed directly (not hidden in a generic type) to handle keys or multiline strings.
  • YAMLItem was a bit more complex than needed. I preferred to remove inheritance and Counted (attach/detach calls were very confusing and opaque to me). Now one class with tags (enum class) and a nested union is used.
    Moreover a std::deque was used to load items - however as only 1 item was loaded ahead, a std::optional is now used instead.

YAML fixes:

  • single quoted strings '...' do not escape \ , \n ....
  • Keys are always strings

With these changes, it is now also very simple to create custom value types (e.g. with std::variant) and replace configurations (if required at all).
I tried to to do a clean and sharp cut, if there are more things you think we should change let me know. The basic parsing logic is still the same.

@DJDavies2
Copy link
Contributor

I'm getting a compile failure with this:

/home/d03/frwd/cylc-run/eckit-80/share/mo-bundle/eckit/src/eckit/parser/ObjectParser.cc:21:1: error: declaration does not declare anything [-fpermissive]
ObjectParser;
^~~~~~~~~~~~
/home/d03/frwd/cylc-run/eckit-80/share/mo-bundle/eckit/src/eckit/parser/JSONParser.cc:21:1: error: declaration does not declare anything [-fpermissive]
GenericJSONParser<parser::ValueBuilder, ObjectParser>;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
gmake[2]: *** [eckit/src/eckit/CMakeFiles/eckit.dir/parser/JSONParser.cc.o] Error 1
gmake[2]: *** Waiting for unfinished jobs....
gmake[2]: *** [eckit/src/eckit/CMakeFiles/eckit.dir/parser/ObjectParser.cc.o] Error 1
/home/d03/frwd/cylc-run/eckit-80/share/mo-bundle/eckit/src/eckit/parser/YAMLParser.cc:21:1: error: declaration does not declare anything [-fpermissive]
GenericYAMLParser<parser::ValueBuilder, ObjectParser>;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
gmake[2]: *** [eckit/src/eckit/CMakeFiles/eckit.dir/parser/YAMLParser.cc.o] Error 1
gmake[1]: *** [eckit/src/eckit/CMakeFiles/eckit.dir/all] Error 2
gmake[1]: *** Waiting for unfinished jobs....

This is with gcc 7.3.0

@geier1993
Copy link
Contributor Author

geier1993 commented Aug 7, 2023

I'm getting a compile failure with this:

/home/d03/frwd/cylc-run/eckit-80/share/mo-bundle/eckit/src/eckit/parser/ObjectParser.cc:21:1: error: declaration does not declare anything [-fpermissive] ObjectParser; ^~~~~~~~~~~~ /home/d03/frwd/cylc-run/eckit-80/share/mo-bundle/eckit/src/eckit/parser/JSONParser.cc:21:1: error: declaration does not declare anything [-fpermissive] GenericJSONParser<parser::ValueBuilder, ObjectParser>; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ gmake[2]: *** [eckit/src/eckit/CMakeFiles/eckit.dir/parser/JSONParser.cc.o] Error 1 gmake[2]: *** Waiting for unfinished jobs.... gmake[2]: *** [eckit/src/eckit/CMakeFiles/eckit.dir/parser/ObjectParser.cc.o] Error 1 /home/d03/frwd/cylc-run/eckit-80/share/mo-bundle/eckit/src/eckit/parser/YAMLParser.cc:21:1: error: declaration does not declare anything [-fpermissive] GenericYAMLParser<parser::ValueBuilder, ObjectParser>; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ gmake[2]: *** [eckit/src/eckit/CMakeFiles/eckit.dir/parser/YAMLParser.cc.o] Error 1 gmake[1]: *** [eckit/src/eckit/CMakeFiles/eckit.dir/all] Error 2 gmake[1]: *** Waiting for unfinished jobs....

This is with gcc 7.3.0

ah yes. I hoped that the new CI will compile some stuff for me without further effort.
Compiled with clang so far...
I may just remove the *Parser.cc files that have basically no content anymore.

@dsarmany dsarmany requested review from simondsmart and danovaro and removed request for simondsmart August 15, 2023 11:40
@geier1993
Copy link
Contributor Author

I still would like to use the CI for testing compilation with different compilers. However on the workflow it says than an reviewer has to approve it - but I think the workflow can be triggered separately from approving the whole PR?

@geier1993 geier1993 closed this Nov 3, 2023
@geier1993 geier1993 deleted the feature/generalize_parser branch November 3, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants