Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <128d057c-0d60-418d-7129-093ea59b8870@gmail.com>
Date:   Sat, 16 Sep 2023 20:25:27 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH] selftests: ALSA: fix warnings in 'test-pcmtest-driver'
To:     Ivan Orlov <ivan.orlov0322@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>, Shuah Khan <shuah@kernel.org>
Cc:     alsa-devel@alsa-project.org, linux-kselftest@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230916-topic-pcmtest_warnings-v1-1-2422091212f5@gmail.com>
 <96ed6e41-65ca-7410-e2d9-78bd18bdf844@gmail.com>
Content-Language: en-US
From:   Javier Carrasco <javier.carrasco.cruz@gmail.com>
In-Reply-To: <96ed6e41-65ca-7410-e2d9-78bd18bdf844@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Ivan,

On 16.09.23 20:05, Ivan Orlov wrote:
> On 9/16/23 19:22, Javier Carrasco wrote:
>> Defining the 'len' variable inside the 'patten_buf' as unsigned
>> makes it more consistent with its actual meaning and the rest of the
>> size variables in the test. Moreover, this removes an implicit
>> conversion in the fscanf function call.
>>
> 
> Considering the fact that the pattern buffer length can't be negative or
> larger that 4096, I really don't think that it is a necessary change.
> 

>> Additionally, remove the unused variable 'it' from the reset_ioctl test.
>>
> 
> Your patches should always contain only one logical change. If you, for
> instance, remove redundant blank lines, combining it with something else
> is fine, but otherwise you should split the changes up.
>
Removing an unused variable is actually removing a blank line from a
logical point of view. Is an extra patch not overkill considering that
it cannot affect the code behavior?
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>> ---
>> Defining the 'len' variable inside the 'patten_buf' as unsigned
>> makes it more consistent with its actual meaning and the rest of the
>> size variables in the test. Moreover, this removes an implicit
>> conversion in the fscanf function call.
>>
>> Additionally, remove the unused variable 'it' from the reset_ioctl test.
> 
> You don't need this text here. Usually it is the place for changelog
> between patch versions if we have more than one version of the patch.
> For instance, if you send a patch V2, it could look like this:
> 
Sorry, this got merged from the cover letter by b4. I will be more
careful next time, thanks!
> Signed-off-by: ...
> ---
> V1 -> V2:
> - Improve something
> - Add something
> 
> So, don't repeat the commit message here :)
> 
> Anyway, great job! I believe this test could be enhanced in lots of
> ways, so I look forward to seeing new patches related to it from you :)
> 
> -- 
> Kind regards,
> Ivan Orlov
Thanks for your feedback and best regards,
Javier Carrasco