eclipse-cyclonedds / cyclonedds-cxx

Other
93 stars 74 forks source link

Feature/generate ostream operators #472

Closed trittsv closed 6 months ago

trittsv commented 7 months ago

fixes #467

This pull requests generates ostream operators for each type.

trittsv commented 6 months ago

Hi @eboasson, could you please have a look at this PR? Would be great πŸ˜ƒ

eboasson commented 6 months ago

I actually started looking at it yesterday afternoon ☺️ The first impression was fine, but there are a few things I want to check/make sure I understand:

You can probably answer some of these straightaway, but as I like to really check any code that goes in you can only save me so much time by providing them πŸ˜‚

trittsv commented 6 months ago

ah great @eboasson, here are my answers:

eboasson commented 6 months ago

Good that I asked: another look at the changes with your answers in mind sufficed πŸ˜€

  • i had to bump some to c++17 where (generator->uses_array || generator->uses_sequence || generator->uses_bounded_sequence || generator->uses_optional) because of the usage of std::optional. Otherwise i would have to split the file rc/ddscxx/include/org/eclipse/cyclonedds/util/ostream_operators.hpp into multiple files where each containing only vector, array, optional etc. None of the examples used optional so it was not necessary so far. But i can split the file if you like to remove the c++17 from the examples?

I think especially for the examples there is no point in avoiding C++17. Just about everyone has access to a C++17 compiler these days, so it is only the older embedded and/or regulated and/or corporate stuff that might not be able to use it for production code. Supporting those cases is nice, but to put effort into examples ... nah.

  • -f sequence-template, -f string-template should work fine. The only thing that could happen is that the custom type may need to provide a ostream operator.

I now see what you mean. I think that's a perfectly sensible expectation.

  • What is the usecase for serdata_print?

The trace files can include the content of samples, that's when it uses serdata_print. In other situations (involving OpenSplice) I've found it to be a very useful feature when trying to understand what exactly is going on in some complicated scenario involving a lot of different things, and so I retained it in Cyclone.

If you do Tracing/Category = trace and look for lines with write_sample or application you can easily find cases where it dumps the contents. If you take the C "hello world", you have the contents, if you use the C++ one, you see no contents.

With your suggested implementation, that should change.

I do consider that separate from this PR: to me, this PR simply makes that possible, and there's no point in delaying the merging of it because it enables this small improvement.