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 17:03:18 UTC
[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
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