[PATCH 00/29] Remove some unused includes

[Adam Tao]

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

> 
> 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.

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

> 
> With regards,
> Daniel