Feature: RDF/XML parser

[mcb5637]

TODO:

• ID reification
• ID exclusivity
• lang tag
• xml literal
• cleanup
• check dependencies
• integrate into IStreamQuadIterator

[liss-h]

first review about architecture and design (not logic).

In general: please split the xml parse into multiple files. One file per state would be good. And please add comments to explain why stuff is happening, what each state is good for and so on. This is very hard to follow even if you know what rdf-xml roughly looks like.

I very much like the idea with the states. Just needs to be organized a little.

[liss-h]

If you split it into multiple files just keep the definitions inline in the structs that makes it easier to read I think

[liss-h]

Thank you for the changes.

As I said before, I do like the idea of the states. However, the concrete implementation is not optimal.

The main concern here is the circular nature of the relationships of the states and the parser. The states can just arbitrarily modify the parser through the given ImplXML &. This is not good for general understandability and correctness.

I suggest a more functional approach with state transitions and secondary actions (something like fn(CurrentState, Input) -> Transition).

The state transitions determine which state the parser will enter next (or that it should pop a state). The secondary actions are emitting triples or errors.

Something like this:

struct Transition {
    using Action = std::variant<StatePop, StatePush, EmitStatement, EmitError>;
    std::vector<Action> actions_;
};

So instead of directly mutating the parser, the states tell the parser what to do (functional/data oriented design). There is a closed set of possible StateTransition actions that each have a clear purpose. (Rust can model this much cleaner than C++, but it still works). Performance is mostly a secondary concern here; maintainability is more important for rdf/xml (due to the complexity of the specification and since there are basically no large rdf/xml datasets).

The whole goal is to get rid of the circular mutability that is currently present and limit what states can do to the parser. That should make it both safer and easier to follow.

For this it makes sense to move some of the functionality out of the ImplXML (e.g. the static functions).

Note: I have not fully thought this through (since it would require me to do the refactor myself), but if you have questions or concerns, feel free to ask.

[liss-h]

Please remove the uses of nested structs (you can keep the ones where ImplXYZ is nested in IStreamQuadIterator for now)

[liss-h]

Thank you for the changes. They really make the code much easier to read. Here is another review, I expect that I will write another (probably final one) after these comments are addressed.

This one is mostly a request for some more example driven documentation and some smaller issues.

[liss-h]

Thank you for all the changes. While this is still complex (because rdf-xml just is complex), I think it is very well structured.

I am just missing a few comments here an there to explain things.

Additionally, you use std::in_place_type_t<XYZ>{} very often, please just use std::in_place_type<XYZ> (without _t and curly braces).

[liss-h]

What happens if I put something in the collection that is not a Description?

[mcb5637]

see added comment

[liss-h]

@nkaralis Do you still have the time to implement the tests for this or should the task be delegated?

[nkaralis]

I will implement them.

[nkaralis]

@mcb5637 Have a look at the commented out tests. In the negative tests, there are multiple that are failing

[liss-h]

Thanks for all the work you put in @mcb5637. I think this is ready to be merged.