On Wed, Sep 15, 2021 at 12:16:04AM -0400, Taylor Blau wrote: > > + /* > > + * If we saw too many prefixes, we must avoid using them at all; as > > + * soon as we have any prefix, they are meant to form a comprehensive > > + * list. > > + */ > > + if (data.prefixes.nr >= TOO_MANY_PREFIXES) > > + strvec_clear(&data.prefixes); > > + > > Great, I find this version even better than what I suggested where the > case where the list already has too many prefixes `continue`s through > the loop before calling strvec_add(). > > Just noting aloud, the `>` part of this conditional will never happen > (since data.prefixes.nr will be at most equal to TOO_MANY_PREFIXES, but > never larger than it). > > Obviously not wrong, but I figured it'd be worth mentioning in case it > caught the attention of other reviewers. Yeah, your analysis is right. Long ago I read some advice (I think from K&R, either the C book or a related article) that suggested always using bounding checks like this rather than equality, because they do the right thing even if the variable ends up with a surprising value. I can certainly see an argument against it (like that if you're so worried about it, maybe you ought to detect it and figure out who is setting the variable to be unexpectedly high). But it seems like a reasonable defensive measure to me (against an off-by-one mis-analysis, or against the earlier code changing in the future). -Peff
