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?