Improve the pretty print for operator<<Frame, operator<<Payload by phoad · Pull Request #504 · rsocket/rsocket-cpp

phoad · 2017-06-06T23:15:52Z

Improve the pretty print code to match the Flag name with the AllowedFlags data member in the Frame classes.

Sample output:
I0606 16:14:25.145932 237998080 RSocketStateMachine.h:144] Out: PAYLOAD[METADANEXT, 1], ([Metadata(8): 'metadata', Data(13): 'Hello Jane 1!']
I0606 16:14:25.146798 237998080 RSocketStateMachine.h:144] Out: PAYLOAD[METADANEXT, 1], ([Metadata(8): 'metadata', Data(14): 'Hello Jane 10!']
I0606 16:14:25.146890 237998080 RSocketStateMachine.h:144] Out: PAYLOAD[LEASE|COMPLE, 1], ([Metadata(0): , Data(0): ]
I0606 16:14:30.147658 237998080 RSocketStateMachine.cpp:445] In: KEEPALIVERESUEN|KALIVR|FOLLOW, 0
I0606 16:14:30.147709 237998080 RSocketStateMachine.cpp:699] Out: KEEPALIVEEMPTY, 0

ragansa

Couple of typos

ragansa · 2017-06-07T00:41:28Z

- ss << delimeter << "FOLLOWS";
+ if (!!(allowedFlags & FrameFlags::RESUME_ENABLE)
+ && !!(frameFlags & FrameFlags::RESUME_ENABLE)) {
+ ss << delimeter << "RESUEN";


might be a typo RESUEN ->RESUME?

ragansa · 2017-06-07T00:41:53Z

+ }
+ if (!!(allowedFlags & FrameFlags::COMPLETE)
+ && !!(frameFlags & FrameFlags::COMPLETE)) { // COMPLETE = 0x40
+ ss << delimeter << "COMPLE";


COMPLE -> COMPLETE?

I was trying to use short version of these enum values. Let me use the whole word version of them. Seems like it will be problem in the future if I use the short versions.

ragansa · 2017-06-07T21:17:10Z

 std::string delimeter = "";
- if (!!(frameFlags & FrameFlags::NEXT)) {
- ss << "NEXT";
+ if (!!(allowedFlags & FrameFlags::IGNORE)


Would it be possible to loop over these flags instead of all the if statements?
You can use the to_string function to print out the flags.

phoad · 2017-06-08T01:41:24Z

Previous commit (in this CR) was printing out multiple values, instead of a single one for FrameFlags' value of 80 / 40.. (Thanks for catching it Scott)
Now, it writes the correct Flag value for the given <Frame, FrameFlags>'s value.

enum clas.. {
RESUME_ENABLE = 0x80,
KEEPALIVE_RESPOND = 0x80,
FOLLOWS = 0x80,
}

Before:
I0606 16:14:30.147658 237998080 RSocketStateMachine.cpp:445] In: KEEPALIVERESUME_ENABLE|KEEPALIVE_RESPOND|FOLLOW, 0
After:
I0607 17:48:27.079867 247017472 RSocketStateMachine.cpp:699] Out: KEEPALIVEKEEPALIVE_RESPOND, 0

As seen KEEPALIVE_RESPOND is the only Flag for Frame_KEEPALIVE, but as it's value was matching with the RESUME_ENABLE, KEEPALIVE_RESPOND and FOLLOW, previous version of this PR was writing all of them. This update fixed it.

Out: PAYLOAD[NEXT|METADATA, 1], ([Metadata(8): 'metadata', Data(13): 'Hello Jane 1!']
Out: PAYLOAD[NEXT|METADATA, 1], ([Metadata(8): 'metadata', Data(14): 'Hello Jane 10!']
Out: PAYLOAD[COMPLETE, 1], ([Metadata(0): , Data(0): ]
In: KEEPALIVEKEEPALIVE_RESPOND, 0
Out: KEEPALIVEEMPTY, 0

ragansa

Looks good to me!

Would it be more manageable if we had the frames implement a common interface and could just have the one std::ostream& operator<<(), and have each frame print out it's own flags?

somasun · 2017-06-08T17:59:56Z

- return os << frame.header_ << ", " << frame.payload_;
+ static const std::vector<std::pair<FrameFlags, std::string>> allowedFlags {
+ {FrameFlags::METADATA, sFrameFlags_METADATA},
+ {FrameFlags::FOLLOWS, sFrameFlags_FOLLOWS} };


Any reason why we are not using the AllowedFlags member variable of the Frame struct (instead of redefining it here) ? Then you could write a simple utility function which takes a list of FrameFlags and returns a string (or vector of strings).
std::ostream& operator<<(std::ostream& os, const Frame_REQUEST_RESPONSE& frame) { return os << frame.header_ << ", " << frame.payload_ << toString(frame.AllowedFlags); }

I think the problem is that the AllowedFlags doesn't tell you which actual flags that frame can have. Just what values of flags that the frame can have. (There is overlap in flag values)
i.e. An AllowedFlags values tells you that the frame can have a flag of value 0x80 but doesn't specify if that means RESUME_ENABLE, KEEPALIVE_RESPOND, or FOLLOWS

Even though the FrameFlags is an enum class, it is used as old C/++ way and OR/AND operations are applied on it to get a bitset, that represents a set of flags. Furthermore, this enum has members which has the same int value. So to be able to differentiate between what's the corresponding Flag enum member for the given int value, we need information from the Frame type. It is not possible to apply switch/case on them.
We write the "flags" when we write the "header". So the information about the flags is existing in the "header" data member. So, this line will not provide the desired result.
return os << frame.header_ << ", " << frame.payload_ << toString(frame.AllowedFlags);
We have a set of flags that are accepted by each Frame type. We should match these flags with the given flag values in the "header.flags". If the frame accepts RESUME_ENABLE, and if the given flag value is 0x80, we should only match with RESUME_ENABLE, but not with KEEPALIVE_RESPOND or FOLLOWS. For this we need to pass information about the frame along with the header to "operator<<" function.
I have tried to do no change in the declaration of the Frame classes and do all the updates in the cpp code.

somasun · 2017-06-08T21:54:49Z

 : "0")
 << (payload.metadata
- ? "): '" + payload.metadata->cloneAsValue().moveToFbString().substr(0, 80).toStdString() + "'"
+ ? "): '" + payload.metadata->cloneAsValue().moveToFbString().substr(0, 40).toStdString() + "'"


please clang-format the code.

somasun · 2017-06-08T21:55:33Z


-std::ostream& operator<<(std::ostream& os, FrameFlags frameFlags) {
- // TODO Match the Flag names with the AllowedFlags in the Frame declarations
+constexpr const char sFrameFlags_EMPTY[] = "EMPTY";


nit. EMPTY can be interpreted as a flag. I would not print any flag.

Yes, you are right. Let me write the 0x00, to state that the flags are empty!

phoad · 2017-06-13T17:01:49Z

@alexmalyshev can you take a look to this PR and merge it?

alexmalyshev · 2017-06-13T21:30:06Z


-std::ostream& operator<<(std::ostream& os, FrameFlags frameFlags) {
- // TODO Match the Flag names with the AllowedFlags in the Frame declarations
+constexpr const char sFrameFlags_EMPTY[] = "0x00";


These should be const char*. Normally char[] is reserved for mutable character buffers.

alexmalyshev · 2017-06-13T21:32:47Z

+constexpr const char sFrameFlags_KEEPALIVE_RESPOND[] = "KEEPALIVE_RESPOND";
+constexpr const char sFrameFlags_FOLLOWS[] = "FOLLOWS";
+constexpr const char sFrameFlags_COMPLETE[] = "COMPLETE";
+constexpr const char sFrameFlags_NEXT[] = "NEXT";


I wouldn't name these sFrameFlags_NEXT. The convention for string literals is kStringLiteralName, and I think we can get away with doing kNext instead of duplicating the sFrameFlags prefix everywhere.

alexmalyshev · 2017-06-13T21:40:27Z

+ std::ostream& os,
+ const std::pair<
+ const FrameHeader&,
+ const std::vector<std::pair<FrameFlags, std::string>>&>&


This is really rough to understand... The second element in this pair is a vector<pair<Flags, std::string>>& which is also inside a wrapping pair<...,...>&. All for the purpose of fitting everything into a single argument for operator<<.
There's no need to use vectors to track which flags are allowed for which frame header. You can write a FrameFlags allowedFlags(FrameType); function and that can use the type in the header. Then have a const char* toString(FrameFlags, FrameType); that'll decide which string to return (need the type to differentiate between flags like FOLLOWS and KEEPALIVE_RESPOND).
I think instead of writing
std::ostream& operator<<(std::ostream&, const std::vector<std::pair<FrameFlags, std::string>>&>&);
I think we can do everything we want with
std::ostream& operator<<(std::ostream&, const FrameHeader&);

Good suggestion. I have also noticed today that we have a matching FrameType per Frame class type. Now its easier to do the thing.

I do not need the FrameFlags information of the Frame class types. Though I need a mapping between each flag type to corresponding string value per Frame class type. I prepared a map for this. I am open for suggestion, though it seemed required such a map to me.

alexmalyshev · 2017-06-13T21:50:32Z


 if(APPLE)
- set(OPENSSL_ROOT_DIR "/usr/local/opt/openssl")
+ set(OPENSSL_ROOT_DIR "/usr/local/opt/openssl;/usr/local/homebrew/opt/openssl")


Should this be part of this PR?

I needed it to build in my box, though it does not need to be here.

alexmalyshev · 2017-06-15T01:16:45Z


-std::ostream& operator<<(std::ostream& os, FrameFlags frameFlags) {
- // TODO Match the Flag names with the AllowedFlags in the Frame declarations
+std::ostream& operator<<(std::ostream& os, std::pair<FrameFlags, FrameType> frameFlagsAndType) {


I still think it's better to make this a function instead of an operator<< overload, i.e. std::ostream& writeFlags(std::ostream&, FrameType, FrameFlags);. Having to make a pair just to pass in two arguments is silly.

Sure. I thought if we write a function we will not be able to write
os << x << y << z;
Let me update the code accordingly.

…'t exist

yschimke · 2017-06-20T07:39:47Z

@fuat this seems like it might be annoying for terminal output when payloads contain binary. Have you considered hex formatting or filtering unprintable characters?

phoad · 2017-06-20T16:14:23Z

@yschimke I will use folly::humanify functionality. Would you like me to print all the data or should I still use substr?

@534

/**

Take a string and "humanify" it -- that is, make it look better.
Since "better" is subjective, caveat emptor. The basic approach is
to count the number of unprintable characters. If there are none,
then the output is the input. If there are relatively few, or if
there is a long "enough" prefix of printable characters, use
backslashify. If it is mostly binary, then simply hex encode.
This is an attempt to make a computer smart, and so likely is wrong
most of the time.
*/

Fuat Geleri added 2 commits June 5, 2017 17:45

Ability to pretty print in/out frames in rsocket-cpp
d4b2215

Make operator<<(Frame), operator<<(Payload) to write prettier output
85cec0c

ragansa reviewed Jun 7, 2017
View reviewed changes

Improve the pretty print for operator<<Frame, operator<<Payload
b1699e4

ragansa reviewed Jun 7, 2017
View reviewed changes

Merge branch 'master' into frameprint
acbebdc

Improve the pretty print for operator<<Frame, operator<<Payload
2f6dd35

ragansa approved these changes Jun 8, 2017
View reviewed changes

somasun requested changes Jun 8, 2017
View reviewed changes

Merge branch 'master' into frameprint
d98aaa9

somasun reviewed Jun 8, 2017
View reviewed changes

somasun approved these changes Jun 8, 2017
View reviewed changes

clang-format Payload.cpp and Frame.cpp
2440b09

phoad closed this Jun 9, 2017

phoad reopened this Jun 9, 2017

lehecka approved these changes Jun 9, 2017
View reviewed changes

phoad closed this Jun 10, 2017

phoad reopened this Jun 10, 2017

phoad closed this Jun 10, 2017

phoad reopened this Jun 10, 2017

phoad closed this Jun 10, 2017

phoad reopened this Jun 10, 2017

Merge branch 'master' into frameprint
c96002c

alexmalyshev requested changes Jun 13, 2017
View reviewed changes

Fuat Geleri added 2 commits June 13, 2017 18:08

Updates, related to Alex's comments
14a0532

Merge branch 'master' into frameprint
1766274

phoad closed this Jun 14, 2017

phoad reopened this Jun 14, 2017

alexmalyshev approved these changes Jun 15, 2017
View reviewed changes

convert std::unordered_map to std::map, as std::hash<FrameType> doesn…
36f0218
…'t exist

phoad closed this Jun 17, 2017

phoad reopened this Jun 17, 2017

phoad merged commit 4407a4a into rsocket:masterJun 17, 2017

phoad deleted the logframe branch June 20, 2017 19:16

Uh oh!

Conversation

phoad commented Jun 6, 2017

Uh oh!

ragansa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoad commented Jun 8, 2017

Uh oh!

ragansa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ragansaJun 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoadJun 9, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoad commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yschimke commented Jun 20, 2017

Uh oh!

phoad commented Jun 20, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ragansaJun 8, 2017•
edited
Loading

phoadJun 9, 2017•
edited
Loading

phoad commented Jun 20, 2017•
edited
Loading