Re: [PATCH v12 1/7] buildtools/chkincs: relax C linkage requirement

["Robin Jarry" <rjarry@redhat.com>]

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 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.

>>> 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.

> Thanks for the review.
>
>>>  endif
>>>
>>>  gen_cpp_files = generator(gen_c_file_for_header,
>>> -- 
>>> 2.43.0
>>