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/generate ostream operators #472

Merged

Conversation

trittsv
Copy link
Contributor

@trittsv trittsv commented Feb 26, 2024

fixes #467

This pull requests generates ostream operators for each type.

@trittsv
Copy link
Contributor Author

trittsv commented Mar 12, 2024

Hi @eboasson, could you please have a look at this PR? Would be great 😃

@eboasson
Copy link
Contributor

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:

  • Why the bump from C++14 to C++17 is needed. (I am fine with it, I think ROS2 did that, too, but I'd like to understand it. I also want to be sure can still build C++11 code with it in a pinch.)
  • Whether it plays nice with the possibility of using some other type than std::vector<T> and similar cases (so -f sequence-template, -f string-template, etc.)
  • Whether it can easily be used to (finally) implement serdata_print
  • What the generated code looks like 🙂

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
Copy link
Contributor Author

trittsv commented Mar 12, 2024

ah great @eboasson, here are my answers:

  • 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?
  • -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.
  • What is the usecase for serdata_print?
    Where can i call the function to test it?
template <typename T>
size_t serdata_print(
  const ddsi_sertype* tpcmn, const ddsi_serdata* dcmn, char* buf, size_t bufsize)
{
  (void)tpcmn;
  (void)dcmn;
  //implementation to follow!!!

  std::stringstream ss;
  ss << static_cast<T*>(dcmn->loan->sample_ptr);
  std::string sample_as_string = ss.str();

  std::cout << "sample printed" << sample_as_string << std::endl;

  if (bufsize > 0)
    buf[0] = 0x0;
  return 0;
}
  • Here are examples of the generated code (the idls from the ddscxx tests)
    generated-code.zip

@eboasson
Copy link
Contributor

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.

@eboasson eboasson merged commit 2de3eea into eclipse-cyclonedds:master Mar 13, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate streaming operators for std::ostream
2 participants