Re: [PATCH] partial cleanup of mdoc(7) arg count validation

[Kristaps Dzonsons <kristaps@bsd.lv>]

> when i ran into a few segfaults, i had a look at argument count
> validation in libmdoc, and it revealed itself as a can of worms.
>
> First, i did some framework improvements:
>   * Added MANDOCERR_ARGCWARN to support real warnings.
>   * Removed CHECK_FATAL because it was unused, and i can't see how
>     a wrong number of arguments on any macro line could ever force
>     us to abort the parser:  For missing arguments, one can assume
>     defaults, and excessive ones can be discarded.
>   * Use the correct level in check_count().
>   * Improve the wording: "more than N children", not "greater than".
>
> Then, the bug fixes that made me look at this:
>   * Don't segfault on empty .Rs.
>   * Don't segfault on empty .Sm and .Db; do the required checks in ebool().
>   * Don't segfault on empty .St; do the required checks in post_st().
>
> While there, i did lots of message level adjustments, all downgrades
> from error to warnings, because none of these is likely to loose relevant
> information.  Some macros had their own validation function anyway,
> so use these to do all checks, making the code easier to follow:
>   * .An -split with additional arguments
>   * .Lb with wrong number of arguments (all checks in post_lb)
>   * .Rs with header arguments
>   * .Rs with an empty body (all checks in post_rs)
>   * posts_text1: These three macros (.In, .Pf, .%U) have more problems:
>     Empty .In and .%U should be removed, and the first word after .Pf
>     should not be parsed, but i don't touch that now.
>   * .sp with excessive arguments
>
> The macros .Dl and .D1 don't need their HEAD children check:
> These macros are blk_part_imp, so they cannot have HEAD children;
> we could make it an assertion, but they have no dedicated validation
> functions, so i guess it's not worth the effort.
>
> After doing all this, i could emove the now unused functions
> eerr_eq0, eerr_eq1, eerr_le1, herr_eq0, herr_ge1,
> only adding ewarn_eq1, ewarn_le1, hwarn_ge1.
>
> The bad news is that eerr_ge1 still needs to be checked.
> Probably, it can be changed to ewarn_ge1; but after finding so many
> issues with the other functions, i'm reluctant to do that blindly,
> so i'm postponing it for now.  It is only used by .Ad .Cd .Er .Fn
> .Ic .%A .%B .%D .%I .%J .%N .%O .%P .%R .%T .%V .Ms .Sx .Sy .Tn .Lk
> .%C .%Q .Vt, so that should be a quick check.  ;-/
>
> OK for what i have so far?

Ingo,

These look fine by me, as the ERROR->WARN downgrade doesn't effect a 
semantic difference.

In general I encourage two functions instead of one (e.g., checking 
whether an argument exist (1) and checking whether it's bool (2)) to 
avoid code duplication.  If your way doesn't bloat the code any, 
however, I'm not opposed to it.

And by all means, feel free to remove those superfluous HEAD checks...

Thanks,

Kristaps
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv