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

Print generated class/struct in C++ #3378

Merged
merged 22 commits into from
Jan 20, 2025

Conversation

bernardnormier
Copy link
Member

@bernardnormier bernardnormier commented Jan 17, 2025

This PR adds printing to generated structs and classes in C++, using operator<<(ostream&, const T&).

Fixes #3324.

The implementation piggy backs on the existing StreamHelpers. It adds a new print function on the StreamHelper template. It also adds an Ice::print function that prints any mapped field type (see StreamHelpers.h).

This implementation detects cycles when printing classes, to avoid stack overflow.

This implementation also supports custom printing with the new ["cpp:custom-print"] metadata for structs and classes. For structs, the application needs to implement operator<<(ostream&, const T&), while for classes, the application needs to implement T::ice_print(ostream&) const.

@bernardnormier bernardnormier marked this pull request as draft January 17, 2025 00:55
@bernardnormier bernardnormier changed the title Initial impl of class/struct printing in C++ Print generated class/struct in C++ Jan 18, 2025
@bernardnormier bernardnormier requested review from pepone, InsertCreativityHere and externl and removed request for pepone January 18, 2025 19:29
@bernardnormier bernardnormier marked this pull request as ready for review January 18, 2025 19:30
@@ -860,6 +861,22 @@ namespace Ice
IceInternal::ReferencePtr _reference;
IceInternal::RequestHandlerCachePtr _requestHandlerCache;
};

ICE_API std::ostream& operator<<(std::ostream&, const ObjectPrx&);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved from ProxyFunctions.h to Proxy.h, because the generated code "sees" Proxy.h but not ProxyFunctions.h


#if __has_include(<span>)
# include <span>
#endif

namespace std
{
// TODO: temporary
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove when #3372 is merged.

testPrint(
byteBoolStruct,
"Test::ByteBoolStruct{myByteSeq = [0x64, 0x96, 0xff], myBoolSeq = [true, false, true]}",
[](ostream& os) { os << hex << showbase; });
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user can easily change the formatting. The Ice "print" functions don't change the format flags of the stream.

using namespace Test;

void
Test::Neighbor::ice_print(ostream& os) const
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example of custom class printing. For custom struct printing, see Ice::Identity, Ice::EncodingVersion.

/// @param os The output stream.
/// @param value The class instance.
/// @return The output stream.
inline std::ostream& operator<<(std::ostream& os, const Value& value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this Ice::Value by reference? Ice always passes Ice::Value using shared_ptr.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't strictly need it, but I think it's nicer and more consistent with mapped exceptions, where (an) operator<< is "equivalent" to ice_print. See #3380.

@@ -13,6 +13,7 @@
[["cpp:include:Ice/Config.h"]]
[["cpp:include:Ice/Comparable.h"]]
[["cpp:include:cstdint"]]
[["cpp:include:ostream"]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this required?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we have above:

[["cpp:no-default-include"]]

which means all C++ includes must be explicit. And since we define structs, we always get:

void ice_printFields(std::ostream& os) const;

deque<const Value*> _localStack;
deque<const Value*>* _currentStack{nullptr}; // may or may not point to _localStack

static const int _cycleCheckIndex;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a thread local?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the API is thread safe. There is still an issue that this would show already printed when same class is being printed by concurrent calls.

Copy link
Member Author

@bernardnormier bernardnormier Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not following.

The index (an int) is global and thread-safe. The data we store (pointer to a deque) is per ostream instance and not shared by all ostream instances. And naturally, a given ostream instance is not supposed to be used concurrently from multiple threads.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind you are right. I was missing that the allocation is done on a per stream with the pword call.

Copy link
Member

@InsertCreativityHere InsertCreativityHere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
I'll let you merge first, then I guess I'll deal with the merge conflicts in my PR,
and remove the stop-gap you put in for enums.


// Our custom implementation of various operator<< for structs defined in Contract.ice

namespace DataStormContract

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if these were in the same order as the Slice definitions, instead of a seemingly random order.
The Slice order:

struct DataSamples
struct ElementInfo
struct TopicInfo
struct TopicSpec
struct ElementData
struct ElementSpec
struct ElementDataAck
struct ElementSpecAck

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I didn't realize this was just moved from TraceUtil.h.

@@ -772,16 +772,18 @@ Slice::Gen::generate(const UnitPtr& p)
}
}

if (!dc->hasMetadata("cpp:no-default-include"))
// Include Ice.h since it was not included in the header.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, as of this PR, the only effect of cpp:no-default-includes is to move
#include <Ice/Ice.h>
from the header file to the source file?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

@@ -2317,8 +2379,7 @@ Slice::Gen::DataDefVisitor::visitClassDefEnd(const ClassDefPtr& p)
C << nl << "return CloneEnabler<" << name << ">::clone(*this);";
C << eb;

const string baseClass = base ? getUnqualified(base->mappedScoped(), scope) : getUnqualified("::Ice::Value", scope);

H << sp;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You added this to fix a formatting bug?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

@bernardnormier bernardnormier merged commit eecd42a into zeroc-ice:main Jan 20, 2025
23 of 24 checks passed
InsertCreativityHere pushed a commit to InsertCreativityHere/compiler-comparison that referenced this pull request Jan 21, 2025
@bernardnormier bernardnormier deleted the ice_print branch March 24, 2025 14:25
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.

Add Support for Printing Generated Slice Types in C++ through operator<<
4 participants