Re: [RFC] Sort #includes in gdb

[Pedro Alves <palves@redhat.com>]

Hi!

On 01/26/2019 03:40 PM, Tom Tromey wrote:

> I did not push this to the buildbot, as I believe it is too large for
> that as well.

ISTR that you could point the bot at some branch instead of a patch?

This would seem particularly useful for this patch, given
the potential for breaking native/host-only code.

> If you want to try building it without running the
> script, it is the branch "submit/sort-includes" in my github.
> 
> I did test that the script produces the same output if run twice.
> (Actually it has a buglet where it still updates the ChangeLog the
> second time, but I didn't feel like fixing this.)
> 
> Let me know what you think.  This does have some possibility of breaking
> the build.

Yeah, there's a likely chance that this will break some native builds -- there are
some headers that are (or used to be) order dependent.  I remember a small number of
patches over the years moving header include order particularly in the
architecture-specific Linux native files, around asm/foo.h, sys/foo.h, headers to
fix the build in some particular kernel/libc version.  ISTR <asm/ptrace.h>
as a particular trouble maker, but I could well be misremembering that one.
I wish I could point at actual code / comments or commits, but I'm not
finding much.  :-/

See this one in gdbserver/linux-arm-low.c at least:

 /* Don't include elf.h if linux/elf.h got included by gdb_proc_service.h.
    On Bionic elf.h and linux/elf.h have conflicting definitions.  */


The fix would be trivial of course, so it doesn't sound very alarming to
me.

BTW, skimming the patch I noticed that the script didn't move
this conditional include (HAVE_GETAUXVAL):

 --- a/gdb/gdbserver/linux-aarch64-ipa.c
 +++ b/gdb/gdbserver/linux-aarch64-ipa.c
 @@ -19,9 +19,11 @@
     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
  
  #include "server.h"
 +
 +#include <elf.h>
  #include <sys/mman.h>
 +
  #include "tracepoint.h"
 -#include <elf.h>
  #ifdef HAVE_GETAUXVAL
  #include <sys/auxv.h>
  #endif

but seems to have handled the conditional include in many other cases.

> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> 
>  #include "defs.h"
> +#include "ada-lang.h"
> +
> +#include <algorithm>
>  #include <ctype.h>
> +#include <sys/stat.h>

What do you think of having separate stanzas for C and C++ system includes?

So we'd have include sections similar to:

 #include "defs.h"
 #include "ada-lang.h"

 #include <ctype.h>
 #include <sys/stat.h>

 #include <algorithm>
 #include <map>
 #include <string>
 #include <vector>

 ...

C++ headers are easily detectable as those without a file extension (no .h).


A minor one: personally, I think I would prefer that all files from the
same directory were sorted together.  I.e., instead of:

 #include "c-lang.h"
 #include "cli/cli-utils.h"
 #include "common/byte-vector.h"
 #include "common/function-view.h"
 #include "common/gdb_vecs.h"
 #include "common/vec.h"
 #include "completer.h"
 #include "dictionary.h"
 #include "symfile.h"
 #include "symtab.h"

This would look clearer / more organized to me:

 #include "cli/cli-utils.h"
 #include "common/byte-vector.h"
 #include "common/function-view.h"
 #include "common/gdb_vecs.h"
 #include "common/vec.h"

 #include "c-lang.h"
 #include "completer.h"
 #include "dictionary.h"
 #include "symfile.h"
 #include "symtab.h"

The above shows directories before non-directory files, because
it seems like the include order goes along with the 
'more general -> more specific', or 'further away -> closer' order.
Something like:

 system headers
 "external" libraries/utilities (libiberty/BFD)
 "internal" libraries/utilities (common / nat / cli / arch)
 gdb/ headers

No firm opinion on comments vs no comments.

Thanks,
Pedro Alves