[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #645: DISPATCH-1516 - Modified log statements to include link identifier. A…

[GitBox <gi...@apache.org>]

kgiusti commented on a change in pull request #645: DISPATCH-1516 - Modified log statements to include link identifier. A…
URL: https://github.com/apache/qpid-dispatch/pull/645#discussion_r356779966
 
 

 ##########
 File path: src/router_core/delivery.c
 ##########
 @@ -373,6 +376,8 @@ void qdr_delivery_increment_counters_CT(qdr_core_t *core, qdr_delivery_t *delive
                 core->modified_deliveries++;
         }
 
+        qd_log(core->log, QD_LOG_DEBUG, delivery->presettled?"Delivery outcome for pre-settled: dlv:%lx link:%"PRIu64" is %s": "Delivery outcome for: dlv:%lx link:%"PRIu64" is %s", (long) delivery,  link->identity, pn_disposition_type_name(outcome));
 
 Review comment:
   I had to read this a couple of times to figure out what's going on.
   I'd avoid using a ?: to pick the format argument - that will work but it's.... clever.
   
   Why not use a format string with a %s entry for the part of the output that varies, i.e.:
   qd_log(..., ..., "Delivery outcome for%s: dlv:%lx link:<etc, etc, etc> is %s", 
               (delivery->presettled) ? " pre-settled" : "",
   ....)
   
   Note the lack of space between "for" and the ":" in the format string and the leading space before "presettled"
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org