Re: [lmi] Include what you use

[Vadim Zeitlin]

On Tue, 26 Oct 2021 23:41:12 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> On 10/26/21 10:29 PM, Vadim Zeitlin wrote:
GC> > 
GC> >  While trying to set up ccache support for the CI builds in an attempt to
GC> > speed them up, I've stumbled upon another item in my TODO list tagged with
GC> > "optimize-build": the one about running https://include-what-you-use.org/
GC> > tool (also known by its "iwyu" abbreviation) on lmi and removing the
GC> > unnecessary headers.
GC> > 
GC> >  It turns out that this tool (and even its very latest version!) is now
GC> > packaged by Debian, so trying it is pretty simple and so I've just done it
GC> > and my first impression is that it somewhat works, even if it's not 100%
GC> > correct. For example [...]
GC> 
GC> Fascinating.
GC> 
GC> > The full include-list for miscellany.hpp:
GC> > #include "config.hpp"
GC> > #include "so_attributes.hpp"  // for LMI_SO
GC> > #include <cctype>             // for tolower, toupper
GC> > #include <climits>            // for UCHAR_MAX
GC> > #include <cstdio>             // for EOF
GC> 
GC> Those comments enable us to understand why it wants to include each
GC> of those headers. That's valuable. In some cases it duplicates what
GC> the source already says:
GC> 
GC> iwyu: #include <climits>            // for UCHAR_MAX
GC> lmi:  #include <climits>                      // UCHAR_MAX
GC> iwyu: #include <cstdio>             // for EOF
GC> lmi:  #include <cstdio>                       // EOF
GC> 
GC> but it's very nice to have an automatically-generated touchstone
GC> to verify that.

 FWIW I thought about converting its output to lmi-compatible form by just
passing it through sed/awk/rakudo/deep-learning-AI to get rid of "for" and
start the comment at 41st column -- this would be simple to do, but mostly
(only?) useful if we wanted to use it automatically, e.g. to compare the
lines output by the tool with the actual lines in the sources.

GC> > miscellany.cpp should add these lines:
GC> > struct tm;
GC> 
GC> I wonder why it would complain here:
GC>     std::tm const*const t1 = std::gmtime(&t0);

 Sorry if I'm misunderstanding your question, but to me it seems clear
enough: the tool wants to forward declare std::tm because it must be
declared here and forward declaration is enough, i.e. it's unnecessary to
include the full <time.h> here.

 Note that this tool works at clang internal representation level and not
just doing some textual analysis or ad hoc C++ parsing, which is how it can
know that the full declaration of std::tm is not needed.

GC> > maybe propose a patch removing the unnecessary includes
GC> Yes, I think we should consider doing this for all files at once.
GC> One large omnibus patch would be easier to handle than separate
GC> patches for hundreds of files.

 OK, I'll continue on this then, thanks!
VZ

pgp1DoJgLF0Bd.pgp
Description: PGP signature