[prev in list] [next in list] [prev in thread] [next in thread]
List: freedesktop-xorg-devel
Subject: Re: [PATCH xts 2/3] xts5: Fix missing variable for format specifier
From: Rhys Kidd <rhyskidd () gmail ! com>
Date: 2016-10-29 22:05:10
Message-ID: CA+iOQUGmvP-Y5o-fMHF2EePqQA=kosUwvrq+YetKA_sxozSJoA () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On 29 October 2016 at 16:45, walter harms <wharms@bfs.de> wrote:
>
>
> Am 29.10.2016 05:37, schrieb Rhys Kidd:
> > XtRemoveEventHandler.c:458:3: warning: format '%d' expects a matching
> 'int' argument [-Wformat=]
> > sprintf(ebuf, "ERROR: Error message handler was invoked %d times");
> > ^
> > XtRemoveEventHandler.c:463:3: warning: format '%d' expects a matching
> 'int' argument [-Wformat=]
> > sprintf(ebuf, "ERROR: Warning message handler was invoked %d times");
> > ^
> > XtRemoveEventHandler.c:540:3: warning: format '%d' expects a matching
> 'int' argument [-Wformat=]
> > sprintf(ebuf, "ERROR: Error message handler was invoked %d times");
> > ^
> > XtRemoveEventHandler.c:545:3: warning: format '%d' expects a matching
> 'int' argument [-Wformat=]
> > sprintf(ebuf, "ERROR: Warning message handler was invoked %d times");
> > ^
> >
> > Signed-off-by: Rhys Kidd <rhyskidd@gmail.com>
> > ---
> > xts5/Xt9/XtRemoveEventHandler.m | 16 ++++++++--------
> > 1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/xts5/Xt9/XtRemoveEventHandler.m b/xts5/Xt9/
> XtRemoveEventHandler.m
> > index 1851ef9..6e6cbbe 100644
> > --- a/xts5/Xt9/XtRemoveEventHandler.m
> > +++ b/xts5/Xt9/XtRemoveEventHandler.m
> > @@ -425,13 +425,13 @@ int invoked = 0;
> > XtAppMainLoop(app_ctext);
> > LKROF(pid2, AVSXTTIMEOUT-2);
> > tet_infoline("TEST: Error message was not emitted");
> > - if (avs_get_event(3) != 0) {
> > - sprintf(ebuf, "ERROR: Error message handler was invoked %d
> times");
> > + if (invoked = avs_get_event(3) != 0) {
> > + sprintf(ebuf, "ERROR: Error message handler was invoked %d
> times", invoked);
> > tet_infoline(ebuf);
> > tet_result(TET_FAIL);
> > }
>
> IMHO this is to complicated. what is wrong with:
>
> invoked = avs_get_event(3);
> if (invoked != 0) {
> sprintf(ebuf, "ERROR: Error message handler was invoked %d
> times", invoked);
> tet_infoline(ebuf);
> tet_result(TET_FAIL);
> }
>
>
>
Hello Walter, thanks for your review.
As an early patch from me, I preferred to keep consistent with the pattern
adopted in this part of the code base -- rather than changing the pattern
via refactoring at the same time as making the bug fix.
e.g. you can see another example here: xts5/XtC/XtSetSelectionTimeout.m
I'd prefer to keep this patch as is in this respect. There's nothing
stopping a follow-up patch making your suggested changes.
>
> > - if (avs_get_event(2) != 0) {
> > - sprintf(ebuf, "ERROR: Warning message handler was invoked
> %d times");
> > + if (invoked = avs_get_event(2) != 0) {
> > + sprintf(ebuf, "ERROR: Warning message handler was invoked
> %d times", invoked);
> > tet_infoline(ebuf);
> > tet_result(TET_FAIL);
> > }
>
>
> btw: this is the same msg als with avs_get_event(3). I have no clue what
> the differenz is
> but maybe it should be reflected in the errmsg.
>
>
There's a subtle difference.
- The errmsg associated with avs_get_event(2) includes the text "*Warning*
message handler...."
- The errmsg associated with avs_get_event(3) includes the text "*Error*
message handler..."
If there is a problem with the existing approach, which is a pattern
throughout this area of the code, then I'd prefer it is addressed in a
subsequent patch.
Accordingly, can I seek your Reviewed-by?
Regards,
Rhys
> just my 2 cents,
>
> re,
> wh
>
>
> > @@ -491,13 +491,13 @@ int invoked = 0;
> > tet_result(TET_FAIL);
> > }
> > tet_infoline("TEST: Error message was not emitted");
> > - if (avs_get_event(3) != 0) {
> > - sprintf(ebuf, "ERROR: Error message handler was invoked %d
> times");
> > + if (invoked = avs_get_event(3) != 0) {
> > + sprintf(ebuf, "ERROR: Error message handler was invoked %d
> times", invoked);
> > tet_infoline(ebuf);
> > tet_result(TET_FAIL);
> > }
> > - if (avs_get_event(2) != 0) {
> > - sprintf(ebuf, "ERROR: Warning message handler was invoked
> %d times");
> > + if (invoked = avs_get_event(2) != 0) {
> > + sprintf(ebuf, "ERROR: Warning message handler was invoked
> %d times", invoked);
> > tet_infoline(ebuf);
> > tet_result(TET_FAIL);
> > }
> _______________________________________________
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
[Attachment #5 (text/html)]
<div dir="ltr">On 29 October 2016 at 16:45, walter harms <span dir="ltr"><<a \
href="mailto:wharms@bfs.de" target="_blank">wharms@bfs.de</a>></span> \
wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div \
class="gmail-h5"><br> <br>
Am 29.10.2016 05:37, schrieb Rhys Kidd:<br>
> XtRemoveEventHandler.c:458:3: warning: format '%d' expects a matching \
'int' argument [-Wformat=]<br> > sprintf(ebuf, "ERROR: Error \
message handler was invoked %d times");<br> > ^<br>
> XtRemoveEventHandler.c:463:3: warning: format '%d' expects a matching \
'int' argument [-Wformat=]<br> > sprintf(ebuf, "ERROR: Warning \
message handler was invoked %d times");<br> > ^<br>
> XtRemoveEventHandler.c:540:3: warning: format '%d' expects a matching \
'int' argument [-Wformat=]<br> > sprintf(ebuf, "ERROR: Error \
message handler was invoked %d times");<br> > ^<br>
> XtRemoveEventHandler.c:545:3: warning: format '%d' expects a matching \
'int' argument [-Wformat=]<br> > sprintf(ebuf, "ERROR: Warning \
message handler was invoked %d times");<br> > ^<br>
><br>
> Signed-off-by: Rhys Kidd <<a \
href="mailto:rhyskidd@gmail.com">rhyskidd@gmail.com</a>><br> > ---<br>
> xts5/Xt9/XtRemoveEventHandler.<wbr>m | 16 ++++++++--------<br>
> 1 file changed, 8 insertions(+), 8 deletions(-)<br>
><br>
> diff --git a/xts5/Xt9/<wbr>XtRemoveEventHandler.m \
b/xts5/Xt9/<wbr>XtRemoveEventHandler.m<br> > index 1851ef9..6e6cbbe 100644<br>
> --- a/xts5/Xt9/<wbr>XtRemoveEventHandler.m<br>
> +++ b/xts5/Xt9/<wbr>XtRemoveEventHandler.m<br>
> @@ -425,13 +425,13 @@ int invoked = 0;<br>
> XtAppMainLoop(app_ctext);<br>
> LKROF(pid2, AVSXTTIMEOUT-2);<br>
> tet_infoline("TEST: Error message was not emitted");<br>
> - if (avs_get_event(3) != 0) {<br>
> - sprintf(ebuf, "ERROR: Error message handler was \
invoked %d times");<br> > + if (invoked = avs_get_event(3) != 0) {<br>
> + sprintf(ebuf, "ERROR: Error message handler was \
invoked %d times", invoked);<br> > \
tet_infoline(ebuf);<br> > tet_result(TET_FAIL);<br>
> }<br>
<br>
</div></div>IMHO this is to complicated. what is wrong with:<br>
<br>
invoked = avs_get_event(3);<br>
if (invoked != 0) {<br>
<span class="gmail-"> sprintf(ebuf, "ERROR: Error message \
handler was invoked %d times", invoked);<br> tet_infoline(ebuf);<br>
tet_result(TET_FAIL);<br>
}<br>
<br>
<br></span></blockquote><div><br></div><div>Hello Walter, thanks for your \
review.<br><br></div><div>As an early patch from me, I preferred to keep consistent \
with the pattern adopted in this part of the code base -- rather than changing the \
pattern via refactoring at the same time as making the bug \
fix.<br><br></div><div>e.g. you can see another example here: \
xts5/XtC/XtSetSelectionTimeout.m<br></div><div><br></div><div>I'd prefer to keep \
this patch as is in this respect. There's nothing stopping a follow-up patch \
making your suggested changes.<br> </div><blockquote class="gmail_quote" \
style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><span class="gmail-"> <br>
> - if (avs_get_event(2) != 0) {<br>
> - sprintf(ebuf, "ERROR: Warning message handler was \
invoked %d times");<br> > + if (invoked = avs_get_event(2) != 0) {<br>
> + sprintf(ebuf, "ERROR: Warning message handler was \
invoked %d times", invoked);<br> > \
tet_infoline(ebuf);<br> > tet_result(TET_FAIL);<br>
> }<br>
<br>
<br>
</span>btw: this is the same msg als with avs_get_event(3). I have no clue what the \
differenz is<br> but maybe it should be reflected in the errmsg.<br>
<br></blockquote><div><br></div><div>There's a subtle \
difference.<br><br></div><div>- The errmsg associated with avs_get_event(2) includes \
the text "*Warning* message handler...."<br></div><div>- The errmsg \
associated with avs_get_event(3) includes the text "*Error* message \
handler..."<br><br></div><div>If there is a problem with the existing approach, \
which is a pattern throughout this area of the code, then I'd prefer it is \
addressed in a subsequent patch.<br><br></div><div>Accordingly, can I seek your \
Reviewed-by?<br><br></div><div>Regards,<br></div><div>Rhys<br></div><div> \
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px \
solid rgb(204,204,204);padding-left:1ex"> just my 2 cents,<br>
<br>
re,<br>
wh<br>
<span class="gmail-"><br>
<br>
> @@ -491,13 +491,13 @@ int invoked = 0;<br>
> tet_result(TET_FAIL);<br>
> }<br>
> tet_infoline("TEST: Error message was not emitted");<br>
> - if (avs_get_event(3) != 0) {<br>
> - sprintf(ebuf, "ERROR: Error message handler was \
invoked %d times");<br> > + if (invoked = avs_get_event(3) != 0) {<br>
> + sprintf(ebuf, "ERROR: Error message handler was \
invoked %d times", invoked);<br> > \
tet_infoline(ebuf);<br> > tet_result(TET_FAIL);<br>
> }<br>
> - if (avs_get_event(2) != 0) {<br>
> - sprintf(ebuf, "ERROR: Warning message handler was \
invoked %d times");<br> > + if (invoked = avs_get_event(2) != 0) {<br>
> + sprintf(ebuf, "ERROR: Warning message handler was \
invoked %d times", invoked);<br> > \
tet_infoline(ebuf);<br> > tet_result(TET_FAIL);<br>
> }<br>
</span>______________________________<wbr>_________________<br>
<a href="mailto:xorg-devel@lists.x.org">xorg-devel@lists.x.org</a>: X.Org \
development<br>
Archives: <a href="http://lists.x.org/archives/xorg-devel" rel="noreferrer" \
target="_blank">http://lists.x.org/archives/<wbr>xorg-devel</a><br>
Info: <a href="https://lists.x.org/mailman/listinfo/xorg-devel" rel="noreferrer" \
target="_blank">https://lists.x.org/mailman/<wbr>listinfo/xorg-devel</a></blockquote></div><br></div></div>
[Attachment #6 (text/plain)]
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic