| Message ID | 20230916-topic-pcmtest_warnings-v1-1-2422091212f5@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | selftests: ALSA: fix warnings in 'test-pcmtest-driver' | expand |
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. > 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: 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
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
On 9/16/23 22:25, Javier Carrasco wrote: > 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? Well, no, it is not, as the line is not blank (nothing except removing a blank line could be considered as blank line removal) :) And an extra patch is not an overkill in case if you are separating logical changes. However, rules may vary from one subsystem to another, so it is up to Shuah to decide take this patch or not. My opinion is that such changes should be separated into different patches.
diff --git a/tools/testing/selftests/alsa/test-pcmtest-driver.c b/tools/testing/selftests/alsa/test-pcmtest-driver.c index 357adc722cba..f0dae651e495 100644 --- a/tools/testing/selftests/alsa/test-pcmtest-driver.c +++ b/tools/testing/selftests/alsa/test-pcmtest-driver.c @@ -13,7 +13,7 @@ struct pattern_buf { char buf[1024]; - int len; + unsigned int len; }; struct pattern_buf patterns[CH_NUM]; @@ -313,7 +313,6 @@ TEST_F(pcmtest, ni_playback) { */ TEST_F(pcmtest, reset_ioctl) { snd_pcm_t *handle; - unsigned char *it; int test_res; struct pcmtest_test_params *params = &self->params;
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. 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. --- tools/testing/selftests/alsa/test-pcmtest-driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) --- base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d change-id: 20230916-topic-pcmtest_warnings-ed074edee338 Best regards,