Re: [PATCH 00/29] Remove some unused includes
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