Re: [PATCH 00/12] Minor fixups for or inspired by clangd and related tools

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 7 Nov 2024 07:47:20 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Nov 06, 2024 at 08:13:29PM +0100, Stefano Brivio wrote:
> > On Wed,  6 Nov 2024 10:25:16 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > I've been experimenting with Zed and clangd recently.  Currently it
> > > generates an enormous number of largely spurious errors and warnings
> > > on the passt code base.  Mostly that's due to its default
> > > configurations not suiting us.  This series adds some configuration
> > > that addresses a number of those warnings, though there remain many
> > > more for now.
> > > 
> > > Some of the warnings also look reasonable, so I have a grab bag of
> > > fixes or workarounds for some of those two.  
> > 
> > This looks good to me. I tested build and functionality on Alpine x86,
> > Debian armhf testing, Debian i686 testing, Debian amd64 testing, Fedora
> > Rawhide x86 with this series plus:
> > 
> > - [PATCH] fwd: Squash different-signedness comparison warning
> > - [PATCH 0/8] Avoid running cppcheck on system headers
> > 
> > on top.
> > 
> > Other than single comments about "[PATCH 0/8] Avoid running cppcheck on
> > system headers", the only issue this series adds is, with clang-tidy 16
> > (current version on Debian testing), a rain of:
> > 
> > /home/sbrivio/passt/.clang-tidy:3:5: error: unexpected scalar
> >     - "clang-diagnostic-*,clang-analyzer-*,*,-modernize-*"
> >     ^
> > 
> > but it's fine, using clang-tidy 18 (on armhf) and clang-tidy 19
> > (everywhere else) fixes that.  
> 
> I thought this might just because I was mixing a string with a list of
> options with a json/yaml list of options.  Alas, no, I think
> clang-tidy 16 only accepts a big string here rather than a yaml list.
> Doing it as a string would prevent the interspersing of the
> explanatory comments, so I don't think it's worth it.
> 
> > There are a bunch of pre-existing cppcheck and clang-tidy warnings that
> > remain after this, and I plan to deal with them:  
> 
> Ok.  Do you need anything from me?

No, no, thanks, I think I already figured out most of those.

> [snip]
> >  cppcheck 2.16 on 32-bit only (!):
> > 
> > --
> > dhcpv6.c:334:14: style: The comparison 'ia_type == 3' is always true. [knownConditionTrueFalse]
> >  if (ia_type == OPT_IA_NA) {
> >              ^
> > dhcpv6.c:306:12: note: 'ia_type' is assigned value '3' here.
> >  ia_type = OPT_IA_NA;
> >            ^
> > dhcpv6.c:334:14: note: The comparison 'ia_type == 3' is always true.
> >  if (ia_type == OPT_IA_NA) {
> >              ^  
> 
> Weird, looks like another false positive, maybe with the same cause as
> that other knownConditionTrueFalse thing we hit.

It could be a similar cause, but the fact that it only happens on armhf
and i686 makes me think it's not really the same.

> > --
> > 
> > 3. clang-tidy 19.1.2 on Alpine x86:
> > 
> > --
> > /home/sbrivio/passt/log.c:216:3: error: misleading indentation: statement is indented too deeply [readability-misleading-indentation,-warnings-as-errors]
> >   216 |                 logfile_rotate_move(fd, now);
> >       |                 ^  
> 
> I think my FALLOC_FL_COLLAPSE_RANGE change should fix this as a side
> effect - the odd indentation is because of the #ifdef cutting off part
> ofthe if.

Oops, sorry, this actually goes away with the second series.

-- 
Stefano