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