On Wed, Mar 18, 2020 at 06:38:49PM -0400, Eric Sunshine wrote:
> To be clear, I wasn't questioning the code structure at all. I was
> specifically referring to the comment talking about "error" when it
> should say "warning" or "possible warning".
>
> Moreover, normally, we use comments to highlight something in the code
> which is not obvious or straightforward, so I was questioning whether
> this comment is even helpful since the code seems reasonably clear.
> And...
OK, I agree with all that. :)
> ...if this is or will become an idiom we want in this codebase, then
> it would be silly to write an explanatory comment every place we
> employ it. Instead, a document such as CodingGuidelines would likely
> be a better fit for such knowledge.
Yeah, that makes sense. If we do use this technique, though, we'll have
to explicitly list "case" lines for the enum values which are meant to
break out to the BUG(). And there it _is_ worth commenting on "yes, we
know about this value but it is not handled here because...". Which is
what you asked for in your original message. :)
Something like:
switch (c) {
case LOFC_BLOB_NONE:
return "blob:none":
..etc...
case LOFC__COUNT:
/* not a real filter type; just a marker for counting the number */
break;
case LOFC_DISABLED:
/* we have no name for "no filter at all" */
break;
}
BUG(...);
-Peff
