Re: [PATCH 2/4] xen/scripts: add cppcheck tool to the xen-analysis.py script

From: Luca Fancellu <Luca.Fancellu@arm.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	Bertrand Marquis <Bertrand.Marquis@arm.com>,
	Wei Chen <Wei.Chen@arm.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Jan Beulich <jbeulich@suse.com>, Julien Grall <julien@xen.org>,
	Wei Liu <wl@xen.org>
Subject: Re: [PATCH 2/4] xen/scripts: add cppcheck tool to the xen-analysis.py script
Date: Thu, 1 Dec 2022 15:34:11 +0000	[thread overview]
Message-ID: <A3204E48-FF22-4275-961C-690F57A8AAEE@arm.com> (raw)
In-Reply-To: <alpine.DEB.2.22.394.2211301145132.4039@ubuntu-linux-20-04-desktop>

Hi Stefano,

>>> 
>>> 
>>>> +            sm_tool_args="n"
>>>> +            ;;
>>>> +        --cppcheck-cmd=*)
>>>> +            CPPCHECK_TOOL="$(eval echo "${OPTION#*=}")"
>>>> +            sm_tool_args="y"
>>>> +            ;;
>>>> +        --cppcheck-html)
>>>> +            CPPCHECK_HTML="y"
>>>> +            sm_tool_args="n"
>>>> +            ;;
>>>> +        --cppcheck-plat=*)
>>>> +            CPPCHECK_PLAT_PATH="$(eval echo "${OPTION#*=}")"
>>>> +            sm_tool_args="n"
>>>> +            ;;
>>>> +        --ignore-path=*)
>>>> +            IGNORE_PATH_LIST="${IGNORE_PATH_LIST} $(eval echo "${OPTION#*=}")"
>>>> +            sm_tool_args="n"
>>>> +            ;;
>>>> +        --)
>>>> +            forward_to_cc="y"
>>>> +            sm_tool_args="n"
>>>> +            ;;
>>>> +        *)
>>>> +            if [ "${sm_tool_args}" = "y" ]; then
>>>> +                CPPCHECK_TOOL_ARGS="${CPPCHECK_TOOL_ARGS} ${OPTION}"
>>>> +            else
>>>> +                echo "Invalid option ${OPTION}"
>>>> +                exit 1
>>> 
>>> It doesn't look like sm_tool_args is really needed? It is only set to
>>> 'y' in the case of --cppcheck-cmd, and in that case we also set
>>> CPPCHECK_TOOL. CPPCHECK_TOOL is the variable used below. Am I missing
>>> something?
>> 
>> We use sm_tool_args to fill CPPCHECK_TOOL_ARGS, basically it’s a state machine where
>> when we find --cppcheck-cmd=<xxx> we expect that every other space separated arguments
>> passed afterwards are the args for cppcheck, so we append to CPPCHECK_TOOL_ARGS
>> until we find an argument that is supposed to be only for this script.
> 
> That seems a bit unnecessary: if the user wants to pass arguments to
> cppcheck, the user would do --cppcheck-cmd="cppcheck arg1 arg2" with ""
> quotes. Doing that should make --cppcheck-cmd="cppcheck arg1 arg2" be
> seen as a single argument from this script point of view. CPPCHECK_TOOL
> would end up being set to "cppcheck arg1 arg2" which is what we want
> anyway? And if we need to distinguish between the cppcheck binary and
> its argument we could use "cut" to extract "cppcheck", "arg1", and
> "arg2" from CPPCHECK_TOOL.  Would that work?
> 

I gave a try for the quotes, the problem is that we need to have quotes in CC=“...”, so adding
quotes also to --cppcheck-cmd= which is inside CC=“...” is preventing the Makefile to work,
I tried escaping etc but I didn’t manage to have it working, so would you agree on keeping it
like that?