[PATCH 00/29] Remove some unused includes

[Daniel P. Berrangé]

On Thu, May 12, 2022 at 01:25:01AM +0800, Adam Tao wrote:
> On 5/12/2022 12:25 AM, Daniel P. Berrangé wrote:
> > On Wed, May 11, 2022 at 09:55:01PM +0800, Peng Liang wrote:
> >> Recently, I update the toolchain in my dev machine from LLVM13 to LLVM14,
> >> and I find that there are many unsed include headers in the libvirt.  So
> >> I try to remove them in this series.
> > 
> > So is clang actually reporting that the headers are unused, or is
> > there some other tool with LLVM14 that is reporting this. I'm
> > basically curious how you go about finding the redundant includes ?
> 
> I use LSP+clangd for my editor and it is a new feature of clangd [1] who
> reports the unused includes. For my editor configuration, it will report a
> hint-level diagnostic for a unused include. But I doesn't find the
> corresponding CLI tool so far, maybe it is clangd itself does the check. So
> I just open the files using my editor then fix the unused diagnostics &
> compile... It's such a waste of time! How I wish there is a CLI tool that can
> do such boring work.
> 
> The feature may be experimental, which need to be enabled manually. And I find
> that the diagnostics for .c file are almost perfect but that for .h file are
> just abort 50-50 correctness.
> 
> [1] https://clangd.llvm.org/design/include-cleaner

That's very interesting, thanks for the pointer.

> > I do wonder if we could automate reporting in CI, but then whether
> > a header is redundant or not, is likely to be platform specific.
> > ie freebsd might need a header but on Linux perhaps not, or vica
> > verca.
> > 
> >> Besides, I also find that:
> >> 1. some header files are not self-contained, which means if you want to
> >>    include one header, you need to include more headers to meet the
> >>    requirements of the declarations in the header you want to include;
> > 
> > This is definitely a bug. We want all our headers to be self-contained
> > and should fix any such problems.
> 
> But I find several .h files which are not self-contained, e.g. src/qemu/qemu_backup.h
> which uses virDomainObj in the declerations but includes nothing itself and
> any .c files including qemu_backup.h need to include several other files.
> 
> > 
> >> 2. some includes in the .h file are not the dependences of the .h file
> >>    (the declaration) but the dependences of the .c file (the
> >>    implementation), maybe it's better to move them to .c file.
> > 
> > Agreed, those would be  better moved into the .c, as it could
> > (theoretically at least) speed up compilation to not huave so
> > many includes visible across the codebase.
> > 
> >> But it will take more time to cleanup.  So I only remove the unused
> >> includs in this series.  Is the community welcome to the removing and
> >> the cleanup I mentioned above?  If so, I'll move on and cleanup more.
> > 
> > Conceptually I think the cleanup is useful. Just have to be careful
> > not to break the code on platforms where different headers might be
> > needed to get the declaration for a given symbol. For example this
> > series breaks on Ubuntu 20.04 and Mingw64:
> > 
> >   https://gitlab.com/berrange/libvirt/-/jobs/2443438983
> >   https://gitlab.com/berrange/libvirt/-/jobs/2443439016
> > 
> > and indeed many other platforms with the same missing geteuid
> > declaration.
> 
> I'm very sorry for that. I only tested the series in my own machine with
> my own build configuration, so maybe the series should be a "RFC" instead
> of a "PATCH"? I will cover more scenarios in the next version.

No problem.

> BTW, how could I trigger the pipeline in my own repo? Just fork & push to
> my own repo can trigger it?

Yes, that's sufficient currently.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|