On 2024-10-03 18:47, Robin Jarry wrote:
Mattias Rönnblom, Sep 20, 2024 at 12:47:
Relax chkincs requirement of all DPDK header files having to contain
'extern "C"'.
Instructing a C++ toolchain to use C linkage is only necessarily if the
header file declares symbols (i.e., functions or global variables).
With this change, chkincs tries to find if any functions or references
to global variables are declared in the header file, and if not, no C
linkage is required.
Signed-off-by: Mattias Rönnblom <mattias.ronnb...@ericsson.com>
--
PATCH v11:
* Detect functions from the Windows POSIX wrappers, to avoid false
positives for redundant 'extern "C"'.
---
Hi Mattias,
I have some remarks on this script. Please see below:
buildtools/chkincs/chkextern.py | 88 +++++++++++++++++++++++++++++++++
buildtools/chkincs/meson.build | 14 +++---
2 files changed, 95 insertions(+), 7 deletions(-)
create mode 100755 buildtools/chkincs/chkextern.py
diff --git a/buildtools/chkincs/chkextern.py b/buildtools/chkincs/
chkextern.py
new file mode 100755
index 0000000000..5374ce1c72
--- /dev/null
+++ b/buildtools/chkincs/chkextern.py
@@ -0,0 +1,88 @@
+#! /usr/bin/env python3
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2024 Ericsson AB
+
+import sys
+import re
+
+def strip_cpp(header):
+ no_cpp = ""
+ header = header.replace("\\\n", " ")
+
+ for line in header.split("\n"):
+ if re.match(r'^\s*#.*', line) is None and len(line) > 0:
+ no_cpp += "%s\n" % line
+
+ return no_cpp
+
+
+def strip_comments(header):
+ no_c_comments = re.sub(r'/\*.*?\*/', '', header, flags=re.DOTALL)
+ no_cxx_comments = re.sub(r'//.*', '', no_c_comments)
+ return no_cxx_comments
+
+
+def strip(header):
+ header = strip_comments(header)
+ header = strip_cpp(header)
+ return header
None of these three strip* functions are used.
Oops.
I think I'll just remove the strip_cpp() function (not sure why I
thought that was ever needed). strip_comments() may be somewhat useful
when looking for the 'extern "C"' string, if for some reason that string
would be in a comment somewhere.
+
+
+def has_extern_c(header):
+ return header.find('extern "C"') != -1
+
+
+def has_vars(header):
+ return re.search(r'^extern\s+[a-z0-9_]+\s.*;', header,
flags=re.MULTILINE) is not None
+
+
+FUNCTION_RES = [
+ r'rte_[a-z0-9_]+\(',
+ r'cmdline_[a-z0-9_]+\(',
+ r'vt100_[a-z0-9_]+\(',
+ r'rdline_[a-z0-9_]+\(',
+ r'cirbuf_[a-z0-9_]+\(',
+ # Windows UNIX compatibility
+ r'pthread_[a-z0-9_]+\(',
+ r'regcomp\(',
+ r'count_cpu\('
+]
+
+
+def has_functions(header):
+ for function_re in FUNCTION_RES:
+ if re.search(function_re, header) is not None:
+ return True
+ return False
+
+
+def has_symbols(header):
+ return has_functions(header) or has_vars(header)
+
+
+def chk_missing(filename):
+ header = open(filename).read()
with open(filename) as f:
header = f.read()
What benefit would that construct be to this program?
+ if has_symbols(header) and not has_extern_c(header):
+ print(filename)
+
+
+def chk_redundant(filename):
+ header = open(filename).read()
with open(filename) as f:
header = f.read()
+ if not has_symbols(header) and has_extern_c(header):
+ print(filename)
Can we rename these functions to check_missing and check_redundant? I
don't think the abbreviation is needed here.
Sure.
(The function name was derived from the script name, which in turn got
its name inspired by chkincs "tool" (directory) name.)
+
+if len(sys.argv) < 3:
+ print("%s missing|redundant <header-file> ..." % sys.argv[0])
+ sys.exit(1)
+
+op = sys.argv[1]
+headers = sys.argv[2:]
+
+for header in headers:
+ if op == 'missing':
+ chk_missing(header)
+ elif op == 'redundant':
+ chk_redundant(header)
I don't see the 'redundant' op being used here.
It's used a patch further down the set.
+ else:
+ print("Unknown operation.")
+ sys.exit(1)
I know it is a simple python script but it would be better to add a
proper main() function and use argparse to handle errors. E.g.:
def main():
parser = argparse.ArgumentParser(description=__doc__)
parser.add_argument("op", choices=("missing", "redundant"))
parser.add_argument("headers", nargs="+")
args = parser.parse_args()
for h in args.headers:
if op == "missing":
chk_missing(h)
elif op == "redundant":
chk_redundant(h)
if __name__ == "__main__":
main()
I don't think you need to bring in the usual Python boiler plate. This
script is not supposed to be invoked directly by a user - it's just an
extension of the meson build scripts.
diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/
meson.build
index f2dadcae18..762f85efe5 100644
--- a/buildtools/chkincs/meson.build
+++ b/buildtools/chkincs/meson.build
@@ -38,13 +38,13 @@ if not add_languages('cpp', required: false)
endif
# check for extern C in files, since this is not detected as an error
by the compiler
-grep = find_program('grep', required: false)
-if grep.found()
- errlist = run_command([grep, '--files-without-match', '^extern
"C"', dpdk_chkinc_headers],
- check: false, capture: true).stdout().split()
- if errlist != []
- error('Files missing C++ \'extern "C"\' guards:\n- ' + '\n-
'.join(errlist))
- endif
+chkextern = find_program('chkextern.py')
+
+missing_extern_headers = run_command(chkextern, 'missing',
dpdk_chkinc_headers,
+ capture: true, check: true).stdout().split()
+
+if missing_extern_headers != []
+ error('Files missing C++ \'extern "C"\' guards:\n- ' + '\n-
'.join(missing_extern_headers))
Instead of just relying on this script output, it would be better if it
exited with a non-zero status when something is wrong. That way you
would not have to capture stdout at all and you could leave meson do the
work.
Sure, but it would be required to invoke the script for every header
file in the tree. Not sure I think that would be a net gain.
Thanks for the review.
endif
gen_cpp_files = generator(gen_c_file_for_header,
--
2.43.0
