Skip to content

Commit

Permalink
cleanup: Ensure we limit the system headers included in .h files.
Browse files Browse the repository at this point in the history
Most system headers contain functions (e.g. `memcpy` in `string.h`)
which aren't needed in our own header files. For the most part, our own
headers should only include types needed to declare our own types and
functions. We now enforce this so we think twice about which headers we
really need in the .h files.
  • Loading branch information
iphydf committed Feb 4, 2022
1 parent cda6c9b commit a767f85
Show file tree
Hide file tree
Showing 7 changed files with 62 additions and 2 deletions.
1 change: 1 addition & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ jobs:
- run: *apt_install
- run: apt-get install -y --no-install-recommends cppcheck g++ llvm-dev
- checkout
- run: other/analysis/check_includes
- run: other/analysis/check_logger_levels
- run: other/analysis/run-check-recursion
- run: other/analysis/run-clang
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
.DS_Store
.DS_Store?
._*
.mypy_cache
.Spotlight-V100
.Trash*
Icon?
Expand Down
57 changes: 57 additions & 0 deletions other/analysis/check_includes
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
#!/usr/bin/env python3
import subprocess
import sys
from typing import Tuple

ALLOWLIST: Tuple[str, ...] = (
# system headers
"pthread.h",
"stdarg.h",
"stdbool.h",
"stddef.h",
"stdint.h",
# toxav stuff, maybe not worth abstracting away
"opus.h",
"vpx/vp8cx.h",
"vpx/vp8dx.h",
"vpx/vpx_decoder.h",
"vpx/vpx_encoder.h",
"vpx/vpx_image.h",
)

out = (subprocess.run(
[
"grep", "-R", "^#include <.*>", "other", "toxav", "toxcore",
"toxencryptsave"
],
check=True,
capture_output=True,
).stdout.decode("utf-8").rstrip())

errors = 0
for line in out.split("\n"):
# other/fun can do what it wants.
if "/fun/" in line:
continue
filename, include = line.split(":", 1)
# We only check headers.
if not filename.endswith(".h"):
continue
# ccompat.h can include some things we don't really want elsewhere.
allowlist = ALLOWLIST
if filename == "toxcore/ccompat.h":
allowlist += ("alloca.h", "malloc.h", "stdlib.h")
header = include[include.index("<") + 1:include.index(">")]
if header not in allowlist:
source = filename[:-2] + ".c"
print(
f"{filename}: includes system header <{header}>, which is not allowed in .h files"
)
print(
" " * len(filename) +
f" consider including it in {source} and exporting an abstraction, instead"
)
errors += 1

if errors:
sys.exit(1)
1 change: 1 addition & 0 deletions toxav/audio.c
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*/
#include "audio.h"

#include <assert.h>
#include <stdlib.h>
#include <string.h>

Expand Down
2 changes: 1 addition & 1 deletion toxav/msi.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
#ifndef C_TOXCORE_TOXAV_MSI_H
#define C_TOXCORE_TOXAV_MSI_H

#include <inttypes.h>
#include <pthread.h>
#include <stdint.h>

#include "audio.h"
#include "video.h"
Expand Down
1 change: 0 additions & 1 deletion toxcore/ccompat.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#ifndef C_TOXCORE_TOXCORE_CCOMPAT_H
#define C_TOXCORE_TOXCORE_CCOMPAT_H

#include <assert.h>
#include <stdbool.h>

bool unused_for_tokstyle(void);
Expand Down
1 change: 1 addition & 0 deletions toxcore/onion_client.c
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
*/
#include "onion_client.h"

#include <assert.h>
#include <stdlib.h>
#include <string.h>

Expand Down

0 comments on commit a767f85

Please sign in to comment.