On 2024-10-04 10:05, Robin Jarry wrote:
Mattias Rönnblom, Oct 04, 2024 at 09:31:
On 2024-10-03 18:47, Robin Jarry wrote:
+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?
open(filename).read() leaks open file descriptors. This construct
ensures the file is closed after reading is complete.
I know what it does. I was asking why it matters in this context. But
sure, I guess you could hit the per-process fd limit before the script
exists.
I understand that this script isn't mission critical, but let's avoid
leaving bad examples in the code that others may follow by accident :)
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.
Same as above, I know it is not a user facing script. But I think it
would be much better to write proper python code to have good examples
for newcomers to follow.
Making small scripts needlessly complicated is not good example, it's a
bad one.
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.
You can store a global exit status in the script and process all headers
before exiting with an error if any.
You will need to give the user a list of offending header files.
Thanks for the review.
endif
gen_cpp_files = generator(gen_c_file_for_header,
--
2.43.0
