| Message ID | 9c10b823-f49b-8c73-2bf8-0fd2b0ba0231@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Markus, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Sat, 22 Apr 2017 23:00:23 +0200 > > * A multiplication for the size determination of a memory allocation > indicated that an array data structure should be processed. > Thus use the corresponding function "devm_kcalloc". I have trouble parsing that sentences. This looks like the typical approach of native german speakers to directly transfer sentence constructions from German to English. Which, in most cases, doesn't work or is just plain confusing. With best wishes, Tobias > * Replace the specification of a data structure by a pointer dereference > to make the corresponding size determination a bit safer according to > the Linux coding style convention. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/dma/s3c24xx-dma.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma/s3c24xx-dma.c b/drivers/dma/s3c24xx-dma.c > index f04c4702d98b..967229829683 100644 > --- a/drivers/dma/s3c24xx-dma.c > +++ b/drivers/dma/s3c24xx-dma.c > @@ -1216,10 +1216,10 @@ static int s3c24xx_dma_probe(struct platform_device *pdev) > if (IS_ERR(s3cdma->base)) > return PTR_ERR(s3cdma->base); > > - s3cdma->phy_chans = devm_kzalloc(&pdev->dev, > - sizeof(struct s3c24xx_dma_phy) * > - pdata->num_phy_channels, > - GFP_KERNEL); > + s3cdma->phy_chans = devm_kcalloc(&pdev->dev, > + pdata->num_phy_channels, > + sizeof(*s3cdma->phy_chans), > + GFP_KERNEL); > if (!s3cdma->phy_chans) > return -ENOMEM; > >
>> * A multiplication for the size determination of a memory allocation >> indicated that an array data structure should be processed. >> Thus use the corresponding function "devm_kcalloc". > I have trouble parsing that sentences. This looks like the typical > approach of native german speakers to directly transfer sentence > constructions from German to English. Which, in most cases, doesn't work > or is just plain confusing. Do you find the following wording better to understand if it would be presented by a script like “checkpatch.pl”? WARNING: Prefer devm_kcalloc over devm_kzalloc with multiply Regards, Markus
SF Markus Elfring wrote: >>> * A multiplication for the size determination of a memory allocation >>> indicated that an array data structure should be processed. >>> Thus use the corresponding function "devm_kcalloc". >> I have trouble parsing that sentences. This looks like the typical >> approach of native german speakers to directly transfer sentence >> constructions from German to English. Which, in most cases, doesn't work >> or is just plain confusing. > > Do you find the following wording better to understand > if it would be presented by a script like “checkpatch.pl”? > > WARNING: Prefer devm_kcalloc over devm_kzalloc with multiply For example. Also I just noticed some previous comment by Krzysztof that pointed that out already. My suggestion: One sentence describing that the current situation is. Another sentence explaining why this is bad/undesirable. Last sentence saying what you're doing to fix this (can sometimes be omitted if it's clear from the diffstat). In this case, the output of the checkpatch script would come in handy. With this, you avoid cramming every information into one long and complicated sentence. With best wishes, Tobias > > > Regards, > Markus >
>> WARNING: Prefer devm_kcalloc over devm_kzalloc with multiply > For example. Also I just noticed some previous comment by Krzysztof that > pointed that out already. > > My suggestion: One sentence describing that the current situation is. Why do you find the sentence for the multiplication information inappropriate (or incomplete) at the moment? > Another sentence explaining why this is bad/undesirable. Which details do you miss here? > In this case, the output of the checkpatch script would come in handy. Its implementation of the check “ALLOC_WITH_MULTIPLY” considers only an other search pattern so far. * Do you find it worthwhile to add a prefix like “devm_” to the used regular expression? * Would like to improve any related scripts for the semantic patch language (Coccinelle software) a bit more? > With this, you avoid cramming every information into one long and > complicated sentence. Thanks for your feedback about other wording preferences. Regards, Markus
SF Markus Elfring wrote: >>> WARNING: Prefer devm_kcalloc over devm_kzalloc with multiply >> For example. Also I just noticed some previous comment by Krzysztof that >> pointed that out already. >> >> My suggestion: One sentence describing that the current situation is. > > Why do you find the sentence for the multiplication information inappropriate > (or incomplete) at the moment? I already explained that. It's a 1:1 translation of a german sentence into English. A native speaker does not write or speak like that. If in doubt, don't use long sentences (with nesting, etc.) at all, and break things down into logical blocks. >> Another sentence explaining why this is bad/undesirable. > > Which details do you miss here? Pretty much everything. >> In this case, the output of the checkpatch script would come in handy. > > Its implementation of the check “ALLOC_WITH_MULTIPLY” considers only an other > search pattern so far. > > * Do you find it worthwhile to add a prefix like “devm_” to the used > regular expression? > > * Would like to improve any related scripts for the semantic patch language > (Coccinelle software) a bit more? I don't understand why you're asking this. I'm talking about the _output_ of checkpatch, not about the script itself. But undoubtedly your patch is motivated by the output of said tool. Hence you should mention that. - Tobias >> With this, you avoid cramming every information into one long and >> complicated sentence. > > Thanks for your feedback about other wording preferences. > > Regards, > Markus >
>> Its implementation of the check “ALLOC_WITH_MULTIPLY” considers only an other >> search pattern so far. >> >> * Do you find it worthwhile to add a prefix like “devm_” to the used >> regular expression? >> >> * Would like to improve any related scripts for the semantic patch language >> (Coccinelle software) a bit more? > I don't understand why you're asking this. Software developers and code reviewers have got different opinions about such checks and their relevance. > I'm talking about the _output_ of checkpatch, This information is clear at first glance. > not about the script itself. But it will not provide the warning you might be looking for while you seem to find my source code analysis approach and notifications improvable. I assume that you might be interested in corresponding extensions for the involved search patterns. > But undoubtedly your patch is motivated by the output of said tool. This tool implemented some checks. > Hence you should mention that. Additional tools take also care for similar software development concerns, don't they? Can it be appropriate to omit the reference to only one Perl script for related use cases? Regards, Markus
diff --git a/drivers/dma/s3c24xx-dma.c b/drivers/dma/s3c24xx-dma.c index f04c4702d98b..967229829683 100644 --- a/drivers/dma/s3c24xx-dma.c +++ b/drivers/dma/s3c24xx-dma.c @@ -1216,10 +1216,10 @@ static int s3c24xx_dma_probe(struct platform_device *pdev) if (IS_ERR(s3cdma->base)) return PTR_ERR(s3cdma->base); - s3cdma->phy_chans = devm_kzalloc(&pdev->dev, - sizeof(struct s3c24xx_dma_phy) * - pdata->num_phy_channels, - GFP_KERNEL); + s3cdma->phy_chans = devm_kcalloc(&pdev->dev, + pdata->num_phy_channels, + sizeof(*s3cdma->phy_chans), + GFP_KERNEL); if (!s3cdma->phy_chans) return -ENOMEM;