* Re: mdocml version 1.12.0 available
[not found] ` <20111011145641.GA25314@britannica.bec.de>
@ 2011-10-13 0:50 ` Ingo Schwarze
2011-10-13 9:55 ` Ulrich Spörlein
0 siblings, 1 reply; 6+ messages in thread
From: Ingo Schwarze @ 2011-10-13 0:50 UTC (permalink / raw)
To: Joerg Sonnenberger; +Cc: tech
Hi Joerg,
Joerg Sonnenberger wrote on Tue, Oct 11, 2011 at 04:56:41PM +0200:
> Attached is the report from clang -analyze. Food for thought ;)
Thanks for sending these.
Below is a patch to improve style at a few places,
none of these are bugs, though.
No regressions according to my tests.
A few more reports are valid, but i suggest to keep those redundant
initializations because it is non-trivial to figure out that all
branches actually set a value, so it's safer to initialize up
front, especially in view of potential future code changes.
Two complaints might be bogus:
> /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:617:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n')
> if (n->prev && MDOC_It == n->prev->tok) {
> ^
> /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:611:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n')
> if (n->prev && MDOC_It == n->prev->tok)
> ^
I have no idea why clang thinks n might be NULL here.
Do you understand that?
Or is this just a false positive?
Yours,
Ingo
Index: man_validate.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/man_validate.c,v
retrieving revision 1.47
diff -u -p -r1.47 man_validate.c
--- man_validate.c 18 Sep 2011 15:54:48 -0000 1.47
+++ man_validate.c 13 Oct 2011 00:04:33 -0000
@@ -209,12 +209,12 @@ check_text(CHKARGS)
{
char *cp, *p;
- cp = p = n->string;
- for (cp = p; NULL != (p = strchr(p, '\t')); p++) {
- if (MAN_LITERAL & m->flags)
- continue;
+ if (MAN_LITERAL & m->flags)
+ return;
+
+ cp = n->string;
+ for (p = cp; NULL != (p = strchr(p, '\t')); p++)
man_pmsg(m, n->line, (int)(p - cp), MANDOCERR_BADTAB);
- }
}
#define INEQ_DEFINE(x, ineq, name) \
@@ -470,7 +470,6 @@ post_UC(CHKARGS)
const char *p, *s;
n = n->child;
- n = m->last->child;
if (NULL == n || MAN_TEXT != n->type)
p = bsd_versions[0];
Index: mdoc_html.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/mdoc_html.c,v
retrieving revision 1.63
diff -u -p -r1.63 mdoc_html.c
--- mdoc_html.c 9 Oct 2011 22:10:51 -0000 1.63
+++ mdoc_html.c 13 Oct 2011 00:04:33 -0000
@@ -499,7 +499,7 @@ mdoc_root_post(MDOC_ARGS)
print_otag(h, TAG_COL, 1, tag);
print_otag(h, TAG_COL, 1, tag);
- t = print_otag(h, TAG_TBODY, 0, NULL);
+ print_otag(h, TAG_TBODY, 0, NULL);
tt = print_otag(h, TAG_TR, 0, NULL);
Index: mdoc_macro.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/mdoc_macro.c,v
retrieving revision 1.69
diff -u -p -r1.69 mdoc_macro.c
--- mdoc_macro.c 18 Sep 2011 15:54:48 -0000 1.69
+++ mdoc_macro.c 13 Oct 2011 00:04:34 -0000
@@ -1428,18 +1428,15 @@ blk_part_exp(MACRO_PROT_ARGS)
/* Clean-up to leave in a consistent state. */
- if (NULL == head) {
+ if (NULL == head)
if ( ! mdoc_head_alloc(m, line, ppos, tok))
return(0);
- head = m->last;
- }
if (NULL == body) {
if ( ! rew_sub(MDOC_HEAD, m, tok, line, ppos))
return(0);
if ( ! mdoc_body_alloc(m, line, ppos, tok))
return(0);
- body = m->last;
}
/* Standard appending of delimiters. */
Index: mdoc_validate.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/mdoc_validate.c,v
retrieving revision 1.95
diff -u -p -r1.95 mdoc_validate.c
--- mdoc_validate.c 18 Sep 2011 15:54:48 -0000 1.95
+++ mdoc_validate.c 13 Oct 2011 00:04:34 -0000
@@ -541,12 +541,11 @@ check_text(struct mdoc *m, int ln, int p
{
char *cp;
- cp = p;
- for (cp = p; NULL != (p = strchr(p, '\t')); p++) {
- if (MDOC_LITERAL & m->flags)
- continue;
+ if (MDOC_LITERAL & m->flags)
+ return;
+
+ for (cp = p; NULL != (p = strchr(p, '\t')); p++)
mdoc_pmsg(m, ln, pos + (int)(p - cp), MANDOCERR_BADTAB);
- }
}
static int
Index: term_ps.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/term_ps.c,v
retrieving revision 1.18
diff -u -p -r1.18 term_ps.c
--- term_ps.c 18 Sep 2011 15:54:48 -0000 1.18
+++ term_ps.c 13 Oct 2011 00:04:34 -0000
@@ -488,8 +488,7 @@ pspdf_alloc(char *outopts)
pagey = 356;
} else if (2 != sscanf(pp, "%ux%u", &pagex, &pagey))
fprintf(stderr, "%s: Unknown paper\n", pp);
- } else if (NULL == pp)
- pp = "letter";
+ }
/*
* This MUST be defined before any PNT2AFM or AFM2PNT
@@ -576,7 +575,7 @@ ps_printf(struct termp *p, const char *f
ps_growbuf(p, PS_BUFSLOP);
pos = (int)p->ps->psmargcur;
- len = vsnprintf(&p->ps->psmarg[pos], PS_BUFSLOP, fmt, ap);
+ vsnprintf(&p->ps->psmarg[pos], PS_BUFSLOP, fmt, ap);
va_end(ap);
--
To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mdocml version 1.12.0 available
2011-10-13 0:50 ` mdocml version 1.12.0 available Ingo Schwarze
@ 2011-10-13 9:55 ` Ulrich Spörlein
2011-10-15 15:02 ` Ingo Schwarze
0 siblings, 1 reply; 6+ messages in thread
From: Ulrich Spörlein @ 2011-10-13 9:55 UTC (permalink / raw)
To: tech
On Thu, 2011-10-13 at 02:50:45 +0200, Ingo Schwarze wrote:
> Hi Joerg,
>
> Joerg Sonnenberger wrote on Tue, Oct 11, 2011 at 04:56:41PM +0200:
>
> > Attached is the report from clang -analyze. Food for thought ;)
>
> Thanks for sending these.
> Below is a patch to improve style at a few places,
> none of these are bugs, though.
> No regressions according to my tests.
>
> A few more reports are valid, but i suggest to keep those redundant
> initializations because it is non-trivial to figure out that all
> branches actually set a value, so it's safer to initialize up
> front, especially in view of potential future code changes.
>
> Two complaints might be bogus:
>
> > /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:617:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n')
> > if (n->prev && MDOC_It == n->prev->tok) {
> > ^
> > /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:611:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n')
> > if (n->prev && MDOC_It == n->prev->tok)
> > ^
>
> I have no idea why clang thinks n might be NULL here.
> Do you understand that?
> Or is this just a false positive?
print_bvspace could have been called with n being NULL. Perhaps the HTML
report is little bit more helpful:
https://www.spoerlein.net/scan-build/mdocml/2011-10-13-1/report-qHe1Tf.html#EndPath
hth
Uli
--
To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mdocml version 1.12.0 available
2011-10-13 9:55 ` Ulrich Spörlein
@ 2011-10-15 15:02 ` Ingo Schwarze
2011-10-15 18:28 ` Joerg Sonnenberger
0 siblings, 1 reply; 6+ messages in thread
From: Ingo Schwarze @ 2011-10-15 15:02 UTC (permalink / raw)
To: tech
Hi Ulrich,
Ulrich Spörlein wrote on Thu, Oct 13, 2011 at 11:55:25AM +0200:
> On Thu, 2011-10-13 at 02:50:45 +0200, Ingo Schwarze wrote:
>> Joerg Sonnenberger wrote on Tue, Oct 11, 2011 at 04:56:41PM +0200:
>>> /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:617:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n')
>>> if (n->prev && MDOC_It == n->prev->tok) {
>>> ^
>>> /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:611:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n')
>>> if (n->prev && MDOC_It == n->prev->tok)
>>> ^
>> I have no idea why clang thinks n might be NULL here.
>> Do you understand that?
>> Or is this just a false positive?
> print_bvspace could have been called with n being NULL.
No, i just checked all callers.
All callers access n before calling print_bvspace,
so it's a false positive.
> Perhaps the HTML
> report is little bit more helpful:
> https://www.spoerlein.net/scan-build/mdocml/2011-10-13-1/report-qHe1Tf.html#EndPath
Not really, but i consider this settled all the same.
Thanks,
Ingo
--
To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mdocml version 1.12.0 available
2011-10-15 15:02 ` Ingo Schwarze
@ 2011-10-15 18:28 ` Joerg Sonnenberger
2011-10-16 10:35 ` Ingo Schwarze
0 siblings, 1 reply; 6+ messages in thread
From: Joerg Sonnenberger @ 2011-10-15 18:28 UTC (permalink / raw)
To: tech
On Sat, Oct 15, 2011 at 05:02:49PM +0200, Ingo Schwarze wrote:
> Hi Ulrich,
>
> Ulrich Spörlein wrote on Thu, Oct 13, 2011 at 11:55:25AM +0200:
> > On Thu, 2011-10-13 at 02:50:45 +0200, Ingo Schwarze wrote:
> >> Joerg Sonnenberger wrote on Tue, Oct 11, 2011 at 04:56:41PM +0200:
>
> >>> /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:617:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n')
> >>> if (n->prev && MDOC_It == n->prev->tok) {
> >>> ^
> >>> /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:611:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n')
> >>> if (n->prev && MDOC_It == n->prev->tok)
> >>> ^
>
> >> I have no idea why clang thinks n might be NULL here.
> >> Do you understand that?
> >> Or is this just a false positive?
>
> > print_bvspace could have been called with n being NULL.
>
> No, i just checked all callers.
> All callers access n before calling print_bvspace,
> so it's a false positive.
That's not clear from the code flow and given that there are explicit
checks for NULL around, it punts. You could make this assumptions clear
with an assert.
Joerg
--
To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mdocml version 1.12.0 available
2011-10-15 18:28 ` Joerg Sonnenberger
@ 2011-10-16 10:35 ` Ingo Schwarze
2011-10-16 11:01 ` Kristaps Dzonsons
0 siblings, 1 reply; 6+ messages in thread
From: Ingo Schwarze @ 2011-10-16 10:35 UTC (permalink / raw)
To: tech
Hi Joerg,
Joerg Sonnenberger wrote on Sat, Oct 15, 2011 at 08:28:16PM +0200:
> On Sat, Oct 15, 2011 at 05:02:49PM +0200, Ingo Schwarze wrote:
>> Ulrich Spörlein wrote on Thu, Oct 13, 2011 at 11:55:25AM +0200:
>>> On Thu, 2011-10-13 at 02:50:45 +0200, Ingo Schwarze wrote:
>>>> Joerg Sonnenberger wrote on Tue, Oct 11, 2011 at 04:56:41PM +0200:
>>>>> /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:617:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n')
>>>>> if (n->prev && MDOC_It == n->prev->tok) {
>>>>> ^
>>>>> /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:611:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n')
>>>>> if (n->prev && MDOC_It == n->prev->tok)
>>>>> ^
>>>> I have no idea why clang thinks n might be NULL here.
>>>> Do you understand that?
>>>> Or is this just a false positive?
>>> print_bvspace could have been called with n being NULL.
>> No, i just checked all callers.
>> All callers access n before calling print_bvspace,
>> so it's a false positive.
> That's not clear from the code flow and given that there are explicit
> checks for NULL around, it punts. You could make this assumptions clear
> with an assert.
Hm, i see, that won't do any harm.
Patch updated; i still see no regressions.
OK?
Ingo
Index: man_validate.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/man_validate.c,v
retrieving revision 1.47
diff -u -p -r1.47 man_validate.c
--- man_validate.c 18 Sep 2011 15:54:48 -0000 1.47
+++ man_validate.c 16 Oct 2011 10:28:55 -0000
@@ -209,12 +209,12 @@ check_text(CHKARGS)
{
char *cp, *p;
- cp = p = n->string;
- for (cp = p; NULL != (p = strchr(p, '\t')); p++) {
- if (MAN_LITERAL & m->flags)
- continue;
+ if (MAN_LITERAL & m->flags)
+ return;
+
+ cp = n->string;
+ for (p = cp; NULL != (p = strchr(p, '\t')); p++)
man_pmsg(m, n->line, (int)(p - cp), MANDOCERR_BADTAB);
- }
}
#define INEQ_DEFINE(x, ineq, name) \
@@ -470,7 +470,6 @@ post_UC(CHKARGS)
const char *p, *s;
n = n->child;
- n = m->last->child;
if (NULL == n || MAN_TEXT != n->type)
p = bsd_versions[0];
Index: mdoc_html.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/mdoc_html.c,v
retrieving revision 1.63
diff -u -p -r1.63 mdoc_html.c
--- mdoc_html.c 9 Oct 2011 22:10:51 -0000 1.63
+++ mdoc_html.c 16 Oct 2011 10:28:55 -0000
@@ -499,7 +499,7 @@ mdoc_root_post(MDOC_ARGS)
print_otag(h, TAG_COL, 1, tag);
print_otag(h, TAG_COL, 1, tag);
- t = print_otag(h, TAG_TBODY, 0, NULL);
+ print_otag(h, TAG_TBODY, 0, NULL);
tt = print_otag(h, TAG_TR, 0, NULL);
Index: mdoc_macro.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/mdoc_macro.c,v
retrieving revision 1.69
diff -u -p -r1.69 mdoc_macro.c
--- mdoc_macro.c 18 Sep 2011 15:54:48 -0000 1.69
+++ mdoc_macro.c 16 Oct 2011 10:28:56 -0000
@@ -1428,18 +1428,15 @@ blk_part_exp(MACRO_PROT_ARGS)
/* Clean-up to leave in a consistent state. */
- if (NULL == head) {
+ if (NULL == head)
if ( ! mdoc_head_alloc(m, line, ppos, tok))
return(0);
- head = m->last;
- }
if (NULL == body) {
if ( ! rew_sub(MDOC_HEAD, m, tok, line, ppos))
return(0);
if ( ! mdoc_body_alloc(m, line, ppos, tok))
return(0);
- body = m->last;
}
/* Standard appending of delimiters. */
Index: mdoc_term.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/mdoc_term.c,v
retrieving revision 1.137
diff -u -p -r1.137 mdoc_term.c
--- mdoc_term.c 20 Sep 2011 09:02:18 -0000 1.137
+++ mdoc_term.c 16 Oct 2011 10:28:56 -0000
@@ -580,6 +580,8 @@ print_bvspace(struct termp *p,
{
const struct mdoc_node *nn;
+ assert(n);
+
term_newln(p);
if (MDOC_Bd == bl->tok && bl->norm->Bd.comp)
Index: mdoc_validate.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/mdoc_validate.c,v
retrieving revision 1.95
diff -u -p -r1.95 mdoc_validate.c
--- mdoc_validate.c 18 Sep 2011 15:54:48 -0000 1.95
+++ mdoc_validate.c 16 Oct 2011 10:28:56 -0000
@@ -541,12 +541,11 @@ check_text(struct mdoc *m, int ln, int p
{
char *cp;
- cp = p;
- for (cp = p; NULL != (p = strchr(p, '\t')); p++) {
- if (MDOC_LITERAL & m->flags)
- continue;
+ if (MDOC_LITERAL & m->flags)
+ return;
+
+ for (cp = p; NULL != (p = strchr(p, '\t')); p++)
mdoc_pmsg(m, ln, pos + (int)(p - cp), MANDOCERR_BADTAB);
- }
}
static int
Index: term_ps.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/term_ps.c,v
retrieving revision 1.18
diff -u -p -r1.18 term_ps.c
--- term_ps.c 18 Sep 2011 15:54:48 -0000 1.18
+++ term_ps.c 16 Oct 2011 10:28:57 -0000
@@ -488,8 +488,7 @@ pspdf_alloc(char *outopts)
pagey = 356;
} else if (2 != sscanf(pp, "%ux%u", &pagex, &pagey))
fprintf(stderr, "%s: Unknown paper\n", pp);
- } else if (NULL == pp)
- pp = "letter";
+ }
/*
* This MUST be defined before any PNT2AFM or AFM2PNT
@@ -576,7 +575,7 @@ ps_printf(struct termp *p, const char *f
ps_growbuf(p, PS_BUFSLOP);
pos = (int)p->ps->psmargcur;
- len = vsnprintf(&p->ps->psmarg[pos], PS_BUFSLOP, fmt, ap);
+ vsnprintf(&p->ps->psmarg[pos], PS_BUFSLOP, fmt, ap);
va_end(ap);
--
To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mdocml version 1.12.0 available
2011-10-16 10:35 ` Ingo Schwarze
@ 2011-10-16 11:01 ` Kristaps Dzonsons
0 siblings, 0 replies; 6+ messages in thread
From: Kristaps Dzonsons @ 2011-10-16 11:01 UTC (permalink / raw)
To: tech
> Index: man_validate.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/mandoc/man_validate.c,v
> retrieving revision 1.47
> diff -u -p -r1.47 man_validate.c
> --- man_validate.c 18 Sep 2011 15:54:48 -0000 1.47
> +++ man_validate.c 16 Oct 2011 10:28:55 -0000
> @@ -209,12 +209,12 @@ check_text(CHKARGS)
> {
> char *cp, *p;
>
> - cp = p = n->string;
> - for (cp = p; NULL != (p = strchr(p, '\t')); p++) {
> - if (MAN_LITERAL& m->flags)
> - continue;
> + if (MAN_LITERAL& m->flags)
> + return;
> +
> + cp = n->string;
> + for (p = cp; NULL != (p = strchr(p, '\t')); p++)
> man_pmsg(m, n->line, (int)(p - cp), MANDOCERR_BADTAB);
> - }
Ingo, Joerg,
I'm fine with these---thanks! (Gah, what was I thinking!?)
Take care,
Kristaps
--
To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-10-16 11:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <4E90B130.3080008@bsd.lv>
[not found] ` <20111011145641.GA25314@britannica.bec.de>
2011-10-13 0:50 ` mdocml version 1.12.0 available Ingo Schwarze
2011-10-13 9:55 ` Ulrich Spörlein
2011-10-15 15:02 ` Ingo Schwarze
2011-10-15 18:28 ` Joerg Sonnenberger
2011-10-16 10:35 ` Ingo Schwarze
2011-10-16 11:01 ` Kristaps Dzonsons
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).