On Wed, Mar 18, 2020 at 6:03 AM Jeff King <peff@xxxxxxxx> wrote: > On Tue, Mar 17, 2020 at 04:53:44PM -0400, Eric Sunshine wrote: > > > + case LOFC__COUNT: > > > + /* > > > + * Include these to catch all enumerated values, but > > > + * break to treat them as a bug. Any new values of this > > > + * enum will cause a compiler error, as desired. > > > + */ > > > > In general, people will see a warning, not an error, unless they > > specifically use -Werror (or such) to turn the warning into an error, > > so this statement is misleading. Also, while some compilers may > > complain, others may not. So, although the comment claims that we will > > notice an unhandled enum constant at compile-time, that isn't > > necessarily the case. > > Yes, but that's the best we can do, isn't it? 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... > But we can't just remove the default case. Even though enums don't > generally take on other values, it's legal for them to do so. So we do > want to make sure we BUG() in that instance. > > This is awkward to solve in the general case[1]. But because we're > returning in each case arm here, it's easy to just put the BUG() after > the switch. Anything that didn't return is unhandled, and we get the > best of both: -Wswitch warnings when we need to add a new filter type, > and a BUG() in the off chance that we see an unexpected value. > > So...I dunno. Worth it as a general technique? ...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.
