eclipse-cyclonedds / cyclonedds-cxx

Other
93 stars 74 forks source link

Moves ostream_operators.hpp include to all implementation files #485

Closed michelle-vu19 closed 17 hours ago

michelle-vu19 commented 5 months ago

Resolves issue #484

michelle-vu19 commented 5 months ago

@trittsv Good morning, can you take a look at this?

trittsv commented 5 months ago

Hey @michelle-vu19, seems that i missed that case described in #484.

I put the include of the ostream_operators.hpp explicitly into the cpp file. Because this file modifies the users std namespace and that is dangerous. So i chose to encapsulate it in the cpp files.

I strongly recommend to keep it in the cpp files.

My suggestion is: Iterate all the included idl files and check if they are using std types and include the ostream_operators.hpp in the cpp files there too.

michelle-vu19 commented 5 months ago

@trittsv Thank you for the suggestion, I can get to this later today.

michelle-vu19 commented 5 months ago

@trittsv From a first glance, it seems like getting information from included IDL files is not something that can be done with the way the generator is set up (not saving information after parsing a single file). Is there a way to get information about an include file without reparsing?

jkav77 commented 2 months ago

I'm running into this problem as well. Manually adding #include <org/eclipse/cyclonedds/util/ostream_operators.hpp> to the files that need it seem to help. I have almost 700 IDL files in my project so this is suboptimal. Is there a fix for this?

michelle-vu19 commented 3 weeks ago

@trittsv @reicheratwork @eboasson Sorry for taking so long to apply this suggestion :) Can I get a review on this? I am currently in the process of compiling CPP types just to verify that this works.

trittsv commented 3 weeks ago

hey @michelle-vu19, looks good, you still need to sign the eclipsefdn/eca to be able to merge

michelle-vu19 commented 3 weeks ago

@trittsv Done!

michelle-vu19 commented 3 weeks ago

Update: my code generated successfully with the new changes :)

michelle-vu19 commented 4 days ago

When can this be merged? :)