Re: [PATCH] xen: Add MISRA support to cppcheck make rule

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jan,

> On 31 May 2022, at 14:50, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 31.05.2022 15:30, Bertrand Marquis wrote:
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -297,6 +297,8 @@ xen/.banner
>> xen/.config
>> xen/.config.old
>> xen/.xen.elf32
>> +xen/cppcheck-misra.txt
>> +xen/cppcheck-misra.json
>> xen/xen-cppcheck.xml
>> xen/System.map
>> xen/arch/x86/boot/mkelf32
> 
> Please can you obey to sorting here, inserting next to
> xen/cppcheck-htmlreport? Seeing that xen/xen-cppcheck.xml was added
> here by you at the same time as xen/cppcheck-htmlreport, may I further
> ask that you move that line to where it belongs as well?

Sure will do.

> 
> Additionally I wonder if you couldn't use just one line, specifying
> xen/cppcheck-misra.* ; this could then also hold for the _clean rule
> addition further down in the patch.

Ok.

> 
>> @@ -703,6 +716,21 @@ cppcheck-version:
>>              exit 1; \
>>      fi
>> 
>> +# List of Misra rules to respect is written inside a doc.
>> +# In order to have some helpful text in the cppcheck output, generate a text
>> +# file containing the rules identifier, classification and text from the Xen
>> +# documentation file. Also generate a json file with the right arguments for
>> +# cppcheck in json format including the list of rules to ignore
> 
> Nit: Missing full stop at the end.

Will fix

> 
>> +# Replace current by goal in the dependency to generate an analysis for all
>> +# rules we would like to respect.
>> +cppcheck-misra.json cppcheck-misra.txt: $(XEN_ROOT)/docs/misra/rules.rst
>> +    $(Q)$(srctree)/tools/convert_misra_doc.py -i $< -o cppcheck-misra.txt \
>> +            -j cppcheck-misra.json
>> +
>> +# Prevent parallel make issues as script is generating both files
>> +cppcheck-misra.json: cppcheck-misra.txt
> 
> With this dependency the earlier rule should not list multiple targets
> (and it generally should not, for not being a pattern rule). If I'm not
> mistaken the way you have it the Python script would be invoked twice,
> and all you prevent is that it is invoked twice in parallel. And then
> please use $@ inside the rule. Additionally, with the script being an
> in-tree one, the rule should also have a dependency on that script
> (such that the targets would be rebuilt if the script alone changes).

I am a bit lost on the $@, previous patch adding cppcheck was changed
to use $(Q) instead. So can you justify this request ?

For the rest, thanks for the finding, I agree and will fix.

Cheers
Bertrand

> 
> Jan
>