[libcamera-devel] Introduce -Wextra-semi to libcamera

[Laurent Pinchart]

Hi Tomi,

On Thu, Oct 22, 2020 at 09:20:42AM +0300, Tomi Valkeinen wrote:
> On 22/10/2020 02:00, Laurent Pinchart wrote:
> 
> >> A matter of taste, and strictly speaking not part of this series, but I think the macros should not
> >> contain the semicolon (e.g. LOG_DECLARE_CATEGORY does), and the semicolon should be where the macro
> >> is used. I believe this is the style followed by the kernel (e.g. EXPORT_SYMBOL, DEFINE_MUTEX, ...).
> >>
> >> I don't know if there are technical reasons either way, but when reading code, my brain flags lines
> >> without semicolon as errors =).
> > 
> > One issue is that sometimes macros expand to functions, and thus don't
> > need to be followed by a semicolon. For instance, we have
> > 
> > #define TEST_REGISTER(klass)                                            \
> > int main([[maybe_unused]] int argc, [[maybe_unused]] char *argv[])      \
> > {                                                                       \
> >         return klass().execute();                                       \
> > }
> > 
> > so writing
> > 
> > TEST_REGISTER(...);
> > 
> > will always be flagged by -Wextra-semi.
> 
> I look at this from a bit different perspective. I think -Wextra-semi is not a very good warning.
> Afaik there are no cases where extra semicolons could cause any code issues (when placed at
> "sensible" places), so extra semicolon is a matter of style, not something to warn about by the
> compiler.

I agree with you. It should be handled by lint tools such as
clang-format, and that's what I would prefer, but it doesn't seem to
support such an option :-S

> I still do like to get rid of extra semicolons, but as it's a matter of style, I'd argue that
> 
> TEST_REGISTER(...);
> 
> does not have an extra semicolon, even if technically, after pre-processing, it has. And, as the
> above line looks like a statement, its style should match that of statements and thus have a semicolon.
> 
> Then again, I can't find any tool that checks it as I describe above.

I think -Wextra-semi (or, better, and equivalent in clang-format when/if
it will support that) is useful to keep the coding style consistent. I
however hear your concerns, and I would scream if I was reading

void Foo::bar(...)
{
	Private *d = LIBCAMERA_D_PTR(Foo)
	d->bar();
}

instead of

void Foo::bar(...)
{
	Private *d = LIBCAMERA_D_PTR(Foo);
	d->bar();
}

For macros use outside function scope, I don't mind that much.
namespaces and functions don't require semicolons, type declarations do.
That's C++, so be it. I would be fine moving the semicolon out of macros
such as LOG_DECLARE_CATEGORY() if there was a consensus for that, but
LOG_DEFINE_CATEGORY() expands to a function, which doesn't require a
semicolon at the end. I think the inconsistency would be quite bad.

-- 
Regards,

Laurent Pinchart