MySQL Bugs: #82649: Include what you use

Bug #82649	Include what you use
Submitted:	19 Aug 2016 9:46	Modified:	24 Aug 2016 16:30
Reporter:	Steinar Gunderson	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: Compiling	Severity:	S3 (Non-critical)
Version:	8.0.1-dmr	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
We should make MySQL #include-what-you-use compliant. Every header file should be separately compilable, ie., you should be able to #include it without first #including everything else. On the flip side, you should not #include what you don't use. #include order should be consistent (C++ system, C system, MySQL headers -- and then alphabetically within each category). I intend to use the IWYU tool for this, but it requires manual cleanup afterwards. Also, every #include file should have an include guard.

Benefits are primarily maintainability, and from experience, compile times tend to go down. (I have projects where compilation time halved as a result of IWYU, although I doubt that will be the case for MySQL.)

How to repeat:
N/A

Suggested fix:
N/A

Posted by developer:
 
Fixed in 8.0.1.

Code cleanup. No changelog entry needed.

Posted by developer:
 
Fixed in 8.0.1.

Code cleanup. No changelog entry needed.

Posted by developer:
 
Hello,

Very good!!!

I have just one question: "#include order should be consistent (C++ system, C system, MySQL headers..." sometimes there are cases where the system headers do provide some stuff only if a given macro is defined and some of our own headers (mysql) defines that macro. But if we first include the system header and then the mysql header that defines the macro it will not work. From this point of view shouldn't we first include our headers, then the system ones? Is there a specific reason to do the opposite?

Posted by developer:
 
An example to illustrate:

our1.h:
-------
#define I_WANT_THE_COOL_FEATURE
#include <systemfoo.h> // will provide the cool feature only if I_WANT_THE_COOL_FEATURE is defined
...
use the cool feature

systemfoo.h
-----------
#ifndef SYSTEMFOO_H // will do nothing if SYSTEMFOO_H is already defined
#define SYSTEMFOO_H
...
#ifdef I_WANT_THE_COOL_FEATURE
provide the cool feature
#endif
...
#endif

our2.h
------
// according to the guidelines include first the system header, then ours
#include <systemfoo.h> // needed for something other than the cool feature
// NOTICE that here SYSTEMFOO_H is defined and the cool feature is not provided
#include "our1.h" // needed for our1 feature
...
use our1 stuff

The above will not compile.

We have no such instances left; things like -D__STDC_LIMIT_MACROS are now given on the command line, not in my_global.h. And it really has to be, because trying to make sure you _don't_ include one header file before you've come to a given #define is incredibly brittle.

Given that your own headers are bound to include system headers, it's impossible to have a consistent order of system headers last, so then it's better to have the system headers first. :-) This is the norm in almost any C program out there.

There's one (obvious) exception to the alphabetization rule; "my_config.h" needs to come before any #if HAVE_* test.

Posted by developer:
 
> We have no such instances left; things like -D__STDC_LIMIT_MACROS are now
> given on the command line, not in my_global.h. And it really has to be,
> because trying to make sure you _don't_ include one header file before you've
> come to a given #define is incredibly brittle.

Ok, I agree that providing the feature macros on the command line is to
be preferred by all means. But I think we are not there yet:

$ git grep -E '#[[:space:]]*define[[:space:]].*(STDC|GNU)'

> Given that your own headers are bound to include system headers, it's
> impossible to have a consistent order of system headers last, so then it's
> better to have the system headers first.
[...]

We are on the same page - provide the macros on the command line,
but out of curiosity - can you elaborate the above, I do not understand
what you mean.

Let's see. I ran your grep and found these (plus some false positives, like #define CLI_MYSQL_REAL_CONNECT STDCALL)

-D_GNU_SOURCE - already set on the command line.
-D__STDC_FORMAT_MACROS - set on the command line.
-D__STDC_LIMIT_MACROS - set on the command line.
-D__STDC_CONSTANT_MACROS - used by rapidjson only, none of our source.
-DSTDC_HEADERS - seems to be set by autoconf, not relevant for system headers.

Did I miss anything?

> We are on the same page - provide the macros on the command line,
> but out of curiosity - can you elaborate the above, I do not understand
> what you mean.

If you try to use

#include "a.h"
#include "b.h"

#include <foo.h>

but a.h does #include <stdio.h>, you've lost the property of “system headers last” -- you get "a.h", <stdio.h>, "b.h", <foo.h> in that order. If you do <foo.h> first, at the very least you can guarantee that no definitions from any of your own headers (a.h, b.h) will pollute that specific header.

Or perhaps more philosophically:

 - System C headers expect a clean slate
 - System C++ headers must be prepared for definitions from system C headers
 - Your own headers must be prepared for definitions from all system headers

Posted by developer:
 
Fixed in 8.0.1.

Code cleanup. No changelog entry needed.