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

[Laurent Pinchart]

Hi Tomi,

On Wed, Oct 21, 2020 at 08:55:28AM +0300, Tomi Valkeinen wrote:
> On 20/10/2020 10:54, Laurent Pinchart wrote:
> > On Tue, Oct 20, 2020 at 02:28:57PM +0900, Hirokazu Honda wrote:
> >> I introduce -Wextra-semi compiler option in this patch series.
> >> The compiler option is currently available with clang.
> >> It enables a developer to get warning if the code contains
> >> unnecessary semicolons. It should help our future development.
> >> Before I add the option to meson.build, I remove existing
> >> unnecessary semicolons when libcamera is built with
> >> -Dandroid=enabled.
> > 
> > Great idea :-) It supersedes one of the fixes I've sent yesterday, which
> > I will drop.
> > 
> > The series looks mostly fine to me. I've flagged a few minor issues
> > which I'll address when applying. There's just one change I think would
> > be good for patch 5/8 that requires updating 6/8 too (or creating a new
> > patch for the src/libcamera/pipeline/ directory). If you agree with the
> > proposal, could you submit a new version of just 5/8 and 6/8 (or a new
> > patch) ?
> 
> 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.

We could consider this (and similar macros) as an exception, but that's
then less consistent.

-- 
Regards,

Laurent Pinchart