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

["Mattias Rönnblom" <hofors@lysator.liu.se>]

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