Fixing includes and Include What You Use (IWYU)

Summary

From Nicolás:

seeing some #include fixes in recent commits... it would be a good idea to adopt the convention that every foo.c includes foo.h first, that way we ensure foo.h compiles on its own and is including everything it's supposed to include I tried to do this and it causes so many build failures due to headers not including what they use 😛

When asked to elaborate further as to what the intended benefit of that actually is:

Example: If I add include foo.h in bar.c, it may not compile, if neither is including glib.h and it turns out to be necessary. Nobody noticed because foo.c included glib.h first. If foo.c includes glib.h and then foo.h, and I remove the glib.h thinking it's unnecessary, it will still build so I will wrongly think it's okay. An automated tool to remove "unnecessary" includes could cause that too. This is especially important if foo.h is a public installed header, someone could make an out-of-tree plugin that uses foo.h, we could remove some includes from foo.h and Wireshark would still build (because bar.c happens to including those first) but we break the plugin Imagine if you include string.h and it fails saying size_t not defined (even though you aren't using size_t), because string.h didn't include stdlib.h

One possible solution is IWYU (from https://include-what-you-use.org/ ):

The main goal of include-what-you-use is to remove superfluous #includes. It does this both by figuring out what #includes are not actually needed for this file (for both .cc and .h files), and replacing #includes with forward-declares when possible.

Running IWYU:

Here's the three-step sequence to run iwyu on the entire Wireshark source tree:

CMAKE_EXPORT_COMPILE_COMMANDS=1 CC="clang" CXX="clang++" cmake ../wireshark
iwyu_tool  -p . > iwyu.out
fix_include < iwyu.out

And then git diff can be run on an individual folder. For example:

git diff wsutil/wmem 

Challenges with IWYU:

IWYU output looks like the following:

/home/user/Desktop/wireshark/ui/qt/sctp_graph_byte_dialog.h should add these lines:
#include <qcontainerfwd.h>  // for QVector
#include <qdialog.h>        // for QDialog
#include <qtmetamacros.h>   // for slots, Q_OBJECT
#include <stddef.h>         // for NULL
class QMouseEvent;
class QWidget;

/home/user/Desktop/wireshark/ui/qt/sctp_graph_byte_dialog.h should remove these lines:
#include <config.h>  // lines 13-13
#include <QDialog>  // lines 18-18

The full include-list for /home/user/Desktop/wireshark/ui/qt/sctp_graph_byte_dialog.h:
#include <glib.h>           // for guint16, guint32
#include <qcontainerfwd.h>  // for QVector
#include <qdialog.h>        // for QDialog
#include <qtmetamacros.h>   // for slots, Q_OBJECT
#include <stddef.h>         // for NULL
#include "cfile.h"          // for capture_file
class QCPAbstractPlottable;  // lines 24-24
class QMouseEvent;
class QWidget;
namespace Ui { class SCTPGraphByteDialog; }  // lines 21-21
struct _sctp_assoc_info;  // lines 26-26

Per Nicolas, Qt recommends using over <qdialog.h>. This uncovered how IWYU seems to have well-known issues with handling QT files:

• https://github.com/include-what-you-use/include-what-you-use/issues/94
• https://github.com/include-what-you-use/include-what-you-use/issues/224

Another question is, should there ever be an 'implicit' include? The only situation that comes to my mind is if you wanted to have a header file that was platform-agnostic, and then the other files include that file, to avoid the need to replicate the #DEFINEs or the like

Additional notes:

Thacker — Yesterday at 10:15 PM

A big extra set of includes comes from packet.h includes frame_data.h, which includes wiretap/wtap.h for the only reason of having a function that takes a pointer to a wtap_rec (which could be defined as typedef struct wtap_rec wtap_rec in that header). So any changes to wtap.h means that some 2600 files are recompiled. Some dissectors do use things from wtap.h, mostly the WTAP_ENCAP defines, a few that use pseudoheader structs. I don't know that any actually use any of the functions defined there.

Nicolás — Yesterday at 10:25 PM

packet.h includes a lot of stuff that "dissectors may need" recently someone did a small change to unit_strings.h and I had to recompile the world because packet.h includes it so I removed it from packet.h... and had to add it to 164 files, not sure if worth it 😅 (I didn't send a MR for that yet) with the right forward-declarations, you could remove frame_data.h from packet.h, packet_info.h, and epan.h, need to re-add frame_data.h and/or wtap.h to several dissectors though