You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2022/03/14 15:39:35 UTC
[GitHub] [qpid-proton] cliffjansen opened a new pull request #362: PROTON-2517: rewind all pn_data about to be used in emitters.h
cliffjansen opened a new pull request #362:
URL: https://github.com/apache/qpid-proton/pull/362
Intercepts all pn_data_t emitter functions and does rewind.
In array case, also points to first node so that pn_data_type and pn_data_get_array work properly.
These are consistent with the old codec.
Not done: save and restore of pn_data_t state at entry and exit on the assumption that the callers of the emmiter functions do not expect this state preserved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] astitcher commented on pull request #362: PROTON-2517: rewind all pn_data about to be used in emitters.h
Posted by GitBox <gi...@apache.org>.
astitcher commented on pull request #362:
URL: https://github.com/apache/qpid-proton/pull/362#issuecomment-1067058621
The save and restore state is needed here because (at least some of) the pn_data_t values used here come from the user and are not internal to the proton implementation. And the user left the pn_data_t in one state and we can't leave it silently in another.
Note that this is true even if the pn_data_t is say an array of symbols representing a capability list - The current proton API just gives the user the pn_data_t to modify and the user the user modifies this pn_data_t but at no point 'gives it back' to proton. This is a really crappy API, but there we are. The user can assume that it can just keep on modifying this pn_data_t, so we do have to maintain the cursor state.
The area of where pn_data_t is exposed for this kind of thing is on my near term list of things to fix!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] asfgit closed pull request #362: PROTON-2517: rewind all pn_data about to be used in emitters.h
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #362:
URL: https://github.com/apache/qpid-proton/pull/362
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] astitcher commented on a change in pull request #362: PROTON-2517: rewind all pn_data about to be used in emitters.h
Posted by GitBox <gi...@apache.org>.
astitcher commented on a change in pull request #362:
URL: https://github.com/apache/qpid-proton/pull/362#discussion_r826180173
##########
File path: c/src/core/emitters.h
##########
@@ -543,22 +544,27 @@ static inline void emit_multiple(pni_emitter_t* emitter, pni_compound_context* c
emit_accumulated_nulls(emitter, compound);
if (!data || pn_data_size(data) == 0) {
pni_emitter_writef8(emitter, PNE_NULL);
Review comment:
Hmm, not related to your changes, but I wonder if this sequence is correct - it looks like in the case where we would emit a null here we shouldn't always emit the accumulated nulls first but just add another one. So I think I got this in the wrong order with a missing early return.
##########
File path: c/src/core/emitters.h
##########
@@ -543,22 +544,27 @@ static inline void emit_multiple(pni_emitter_t* emitter, pni_compound_context* c
emit_accumulated_nulls(emitter, compound);
if (!data || pn_data_size(data) == 0) {
pni_emitter_writef8(emitter, PNE_NULL);
- } else if (pn_data_type(data) == PN_ARRAY) {
- switch (pn_data_get_array(data)) {
- case 0:
- pni_emitter_writef8(emitter, PNE_NULL);
- break;
- case 1:
- pn_data_enter(data);
- pn_data_narrow(data);
- pni_emitter_data(emitter, data);
- pn_data_widen(data);
- break;
- default:
+ } else {
+ // Rewind and point to first node so data type is defined.
+ pn_data_rewind(data);
+ pn_data_next(data);
+ if (pn_data_type(data) == PN_ARRAY) {
+ switch (pn_data_get_array(data)) {
+ case 0:
+ pni_emitter_writef8(emitter, PNE_NULL);
+ break;
+ case 1:
+ pn_data_enter(data);
+ pn_data_narrow(data);
+ pni_emitter_data(emitter, data);
+ pn_data_widen(data);
+ break;
+ default:
+ pni_emitter_data(emitter, data);
+ }
+ } else {
Review comment:
Rather than re indent everything I'd prefer there to be an early return and duplicate compound->count++ in the null case with no else block.
I should probably written it this way initially.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org