Observations during our review

[dkeeney]

I am working with the open source htm-community for numenta, working on AI software algorithms. Our software is a set of Python apps with an C++ extension library. We build packages for this library for Windows (MSVC 2017), Linux (Ubuntu), and OSx using CMake. Working with at least C++11 compilers. Our CI builds and runs unit tests for each platform. One of those also builds and tests in debug mode to make sure we don't miss any errors that this might uncover.

We use Yaml to specify algorithm parameters. We have been using yaml-cpp but it can have an issue in debug mode on ubuntu so we were looking for an alternative. The features you list and the speed measurements were exactly what we were looking for. I made a list of the issues that we found while evaluating RapidYaml.

• We do automatic downloads of dependencies as part of the install and found that we had to download 4 separate files and make sure they were stuck in the right place in the source tree. I noticed that you had links in there but the master.zip downloaded from github does not follow those. I recommend that you consolidate these into one repository.
• You do not have a released version so we had to download directly from the master. The master will change and when it does you could break our release. If you had a release, even if not complete, would allow our release to remain stable while we verify your next release still works with our code.
• The downloaded package required 3 separate include directories. I figured out what they needed to be but some documentation would have been helpful. Combining into one repository would help reducing this to one Include Directory.
• No convenient way to convert between std::string and your internal string format. I understand why you use the internal string format and that is cool because that is where you get your speed. But you might provide something like this for the rest of us that must use STL

  // The convert to and from the special string types used by ryml
  static  csubstr _S(const std::string &s) { return to_csubstr(s.c_str()); }
  static std::string S_(csubstr s) { return std::string(s.data(), s.size()); }

• We need an iterator (both const and non-const) for map and sequences. I could not get your begin(), end() to compile and could not find a cbegin(),cend(). I was able to write my own using the NodeRef::first_child(), last_child(), next_sibling(), etc. But you should check the iterators you provide.
• Exceptions. This is the killer. We live and die by C++ exceptions and an exception does not necessarily mean that the program needs to die. For example, our unit tests contain negative as well as positive checks. So it will expect to find an exception in some cases. stopping at a break-point is not an option, even in debug mode. I was able to turn on exception handling in your library. It created the exception ok but then the program aborted. You also have a callback facility that can be called on an error. So I setup a callback handler and had that bootstrap back into C++ where it could throw an exception. That did not work either. I suspect that the internal structures my be in an undefined state at the point when the error was detected and the exception thrown. I tried this on Windows MSVC 2019 which I know to be the most picky about exceptions. I did not try this on the other two platforms.

So the conclusion is that we will have to wait until this package matures a little more and until we can be sure that when errors occur in your C code action is taken to insure that data structures are in a known state and a status code can be returned or a C++ std::exceptions can be thrown.

At this point, being thread-safe is not an issue but in the near future it may become important. I did not see any logic to insure thread safety in your code. It is also likely that we will need to have more than one Yaml tree open at the same time in one program. I did see a few globals that might cause problems in scenarios like this.

Our open-source repo is at https://github.com/htm-community/htm.core in case you are interested.

[parihaaraka]

joining to @dkeeney : tons of warnings like "empty expression statement has no effect; remove unnecessary ';' to silence this warning" may be fixed with macroses like this:

#define ev_idle_init(ev,cb)  do { ev_init ((ev), (cb)); ev_idle_set ((ev)); } while (0)

[dkeeney]

??? don't care about warnings. What we need is the ability to turn all hard errors into C++ exceptions.

[biojppm]

@dkeeney Thanks for the detailed review! I've been nuclear busy and somehow missed it; I've only seen it now. Will respond ASAP; thanks for the patience.

[biojppm]

@parihaaraka thanks for reporting. opened new issue to track that.

[biojppm]

@dkeeney Thanks again for the considerate review. In the coming days I will address the items individually by topic.

To avoid a sprawling thread and make it easier to track individual issues, if you feel the need to further discuss any of the topics, I invite you to open separate issues.

Right, let's do it.

[biojppm]

@dkeeney Addressing exceptions first.

The error behavior you have observed (with debug breakpoints and all) is just the default error callback. Note this is set up as a callback precisely so that you, the client, can control what happens when an error occurs. You can even leave the default callback out of the compilation using a macro, RYML_NO_DEFAULT_CALLBACKS; see the c4/ryml/common.hpp header and source file.

More on point, you can easily set ryml to use an error callback of your own, and throw an exception from there; that way you will get back at your original calling site (or above), where you can catch the exception thrown from your callback. You can see a working example of such an approach in the unit tests for the c4core library; later on I'll provide an equivalent one for ryml itself. Also, note that this unit test also works with VS2019.

So this addresses the use of exceptions.

Having said that, there is still an elephant in the room, and it is neither pretty nor small. Currently, ryml provides no guarantees about the correct state of its objects after an error occurs. Making sure that happens is not a trivial task, and is something likely to make the code more complicated and less efficient.

Or maybe not. If you could provide me with an example use case I will start reflecting on it.

Assuming that this is mostly a parse problem; the emitting problem is much better defined as you have full control over the types which are to be emitted. Something that I imagine can be done is returning the tree/index of the "closest" parent/sibling nodes which were parsed, together with the YAML buffer source point at which the error was detected. So only the "current" node would need to be rolled back. This would make the tree consistent up to the error point.

But the talk is different for the parser. It is a complicated state machine, and at first thought it figures as something very difficult to successfully roll back.

If you want to follow up on this issue of exceptions/consistency, let's do so on a separate issue.

[biojppm]

@dkeeney Do you still have an interest in this? If you do please reply; otherwise I guess I can close this.

[dkeeney]

you can close this item.

On Mon, Jan 20, 2020 at 12:47 PM jpmag notifications@github.com wrote:

@dkeeney https://github.com/dkeeney Do you still have an interest in this? If you do please reply; otherwise I guess I can close this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/biojppm/rapidyaml/issues/12?email_source=notifications&email_token=ABIFV74PEZVHNYQJ2BGP4XTQ6YEVXA5CNFSM4IXV7BFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJNY5OY#issuecomment-576425659, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABIFV72MNVM74QNMJVGCROLQ6YEVXANCNFSM4IXV7BFA .