RFC: self-contained headers

[James Peach <jp...@apache.org>]

Hi all,

I’ve posted a pull request <https://github.com/apache/trafficserver/pull/770> that integrates clang-tidy into the build. The usual way to do this is to use the bear tool to build a JSON compilation database, however that has a couple of drawbacks. First, dynamic library preloading is disabled by default on OS X and some Linux configurations. Second, the database only contains files you actually build (ie. no header files).

The approach in the PR is to add a ‘tidy’ target to all the makefiles which uses the locally defined compiler arguments to tidy every source file, including headers. Now, this goes horribly wrong in some places (eg. iocore) because our headers are not self-contained. clang-tidy cannot parse the incomplete header file and does not make any of the changes we want. If we force it to continue despite the errors, it can make incorrect source changes.

So, should we try to make all our headers self-contained so that clang-tidy can process them? This probably would involve somewhat extensive header refactoring. I’d appreciate any feedback (and volunteers!) …

cheers,
James

Re: RFC: self-contained headers

[James Peach <ja...@me.com>]

> On Jul 2, 2016, at 5:11 PM, Alan Carroll <so...@yahoo-inc.com.INVALID> wrote:
> 
> Yes, it's unclear to me what "self contained" means. Obviously we can't have exactly one header file for Traffic Server, so include must be allowed. What property of the current header file structure doesn't include sufficiently to make clang-tidy happy? 

By self-contained I mean that each header #includes all the others that it depends on. For example, if you have to #include a bunch of other headers in your .cc before you include foo.h, then foo.h is not self-contained. It would be self-contained if it #included everything that it needed.

One specific example. I_Lock.h is not self contained because it doesn’t #include the headers needed for RefCountObj, Ptr and a bunch of other stuff. To make clang-tidy changes to a header file, you would do this:

/opt/clang/bin/clang-tidy -fix -fix-errors I_Lock.h -- c++ -DHAVE_CONFIG_H -I. -I../../lib  -I../../lib -I../../lib/records -Ddarwin -DDEBUG -D_DEBUG -D_LARGEFILE64_SOURCE=1 -D_COMPILE64BIT_SOURCE=1 -D_GNU_SOURCE -D_REENTRANT -D__STDC_LIMIT_MACROS=1 -D__STDC_FORMAT_MACROS=1 -I/usr/local/Cellar/openssl/1.0.2h_1/include -DOPENSSL_NO_SSL_INTERN -I/System/Library/Frameworks/Tcl.framework/Versions/8.5/Headers -I/usr/local/include -I/usr/local/Cellar/pcre/8.39/include  -O0 -std=c++11 -g -pipe -Wall -Wno-deprecated-declarations -Qunused-arguments -Werror -Wno-invalid-offsetof -mcx16 -x c++

Note that the header itself is passes to clang-tidy, so for the code in the header to be valid in this context, clang-tidy needs to see all the prerequisites.

J

Re: RFC: self-contained headers

[Alan Carroll <so...@yahoo-inc.com.INVALID>]

Yes, it's unclear to me what "self contained" means. Obviously we can't have exactly one header file for Traffic Server, so include must be allowed. What property of the current header file structure doesn't include sufficiently to make clang-tidy happy? 

Re: RFC: self-contained headers

[Leif Hedstrom <zw...@apache.org>]

> On Jul 2, 2016, at 11:39 AM, James Peach <jp...@apache.org> wrote:
> 
> Hi all,
> 
> I’ve posted a pull request <https://github.com/apache/trafficserver/pull/770> that integrates clang-tidy into the build. The usual way to do this is to use the bear tool to build a JSON compilation database, however that has a couple of drawbacks. First, dynamic library preloading is disabled by default on OS X and some Linux configurations. Second, the database only contains files you actually build (ie. no header files).
> 
> The approach in the PR is to add a ‘tidy’ target to all the makefiles which uses the locally defined compiler arguments to tidy every source file, including headers. Now, this goes horribly wrong in some places (eg. iocore) because our headers are not self-contained. clang-tidy cannot parse the incomplete header file and does not make any of the changes we want. If we force it to continue despite the errors, it can make incorrect source changes.
> 
> So, should we try to make all our headers self-contained so that clang-tidy can process them? This probably would involve somewhat extensive header refactoring. I’d appreciate any feedback (and volunteers!) …


What does this imply? A header file (.h file) can not include another .h file that’s in the ATS tree?

— leif