Re: [PATCH] cpumask: add a few comments of cpumask functions

[Alex Shi]

On 05/28/2012 08:47 PM, Srivatsa S. Bhat wrote:

> On 05/28/2012 02:32 PM, Alex Shi wrote:
> 
>> Current few cpumask function purpose are not quite clear. Stupid
>> user like myself need to dig into details for clear function
>> purpose and return value.
> 
> 
> You can just see how it is used elsewhere and figure it out ;-)
> Anyway, in principle, I don't have any objections to adding comments
> that are actually helpful. But I don't think all the comments this
> patch adds fall into that category..
> 
>> Add few explanation for them is helpful.
>>
>> Signed-off-by: Alex Shi <alex.shi@xxxxxxxxx>
>> ---
>>  include/linux/cpumask.h |    6 ++++++
>>  1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
>> index a2c819d..8436e61 100644
>> --- a/include/linux/cpumask.h
>> +++ b/include/linux/cpumask.h
>> @@ -271,6 +271,7 @@ static inline void cpumask_clear_cpu(int cpu, struct cpumask *dstp)
>>   * cpumask_test_cpu - test for a cpu in a cpumask
>>   * @cpu: cpu number (< nr_cpu_ids)
>>   * @cpumask: the cpumask pointer
>> + * return 1 if the 'cpu' is in the old 'cpumask', otherwise return 0
>>   *
> 
> 
> s/return/Returns
> 
> What do you mean by "old" cpumask?




Thanks for comments!
Should be "the old bitmap of cpumask"?

> 
>>   * No static inline type checking - see Subtlety (1) above.
>>   */
>> @@ -281,6 +282,7 @@ static inline void cpumask_clear_cpu(int cpu, struct cpumask *dstp)
>>   * cpumask_test_and_set_cpu - atomically test and set a cpu in a cpumask
>>   * @cpu: cpu number (< nr_cpu_ids)
>>   * @cpumask: the cpumask pointer
>> + * return 1 if the 'cpu' is in the old 'cpumask', otherwise return 0
>>   *
> 
> 
> "Test and Set" already has a very well-defined and well-known meaning.. Not sure if the
> comment adds any extra value..




Yes, but it is still helpful for newbie. :)

> 
>>   * test_and_set_bit wrapper for cpumasks.
>>   */
>> @@ -293,6 +295,7 @@ static inline int cpumask_test_and_set_cpu(int cpu, struct cpumask *cpumask)
>>   * cpumask_test_and_clear_cpu - atomically test and clear a cpu in a cpumask
>>   * @cpu: cpu number (< nr_cpu_ids)
>>   * @cpumask: the cpumask pointer
>> + * return 1 if the 'cpu' is in the old 'cpumask', otherwise return 0
>>   *
> 
> 
> Same here. Pretty well known meaning.
> 
>>   * test_and_clear_bit wrapper for cpumasks.
>>   */
>> @@ -324,6 +327,7 @@ static inline void cpumask_clear(struct cpumask *dstp)
>>   * @dstp: the cpumask result
>>   * @src1p: the first input
>>   * @src2p: the second input
>> + * if *dstp is empty, return 0, otherwise return 1
> 
> 
> I don't think anybody would be interested in the return value of this function!
> The functionality (rather than the return value) is what is more interesting
> and useful, and is already well-documented.




:) it is true none is using the return value, but why not to have a comments 
if we have this return value.

> 
>>   */
>>  static inline int cpumask_and(struct cpumask *dstp,
>>  			       const struct cpumask *src1p,
>> @@ -365,6 +369,7 @@ static inline void cpumask_xor(struct cpumask *dstp,
>>   * @dstp: the cpumask result
>>   * @src1p: the first input
>>   * @src2p: the second input
>> + * if *dstp is empty, return 0, otherwise return 1
> 
> 
> Same here. Functionality is more interesting.
> 
>>   */
>>  static inline int cpumask_andnot(struct cpumask *dstp,
>>  				  const struct cpumask *src1p,
>> @@ -414,6 +419,7 @@ static inline bool cpumask_intersects(const struct cpumask *src1p,
>>   * cpumask_subset - (*src1p & ~*src2p) == 0
>>   * @src1p: the first input
>>   * @src2p: the second input
>> + * return 1 if the *src1p is the subset of *src2p, otherwise return 0
> 
> 
> s/return/Returns
> (I think the function name itself is pretty descriptive, but anyway...)




Just this function make me confusing, because I missed the '== 0' in 
"*src1p & ~*src2p) == 0", than I thought scr1p should be the mother mask
 and scr2p is daughter. :)

> 
>>   */
>>  static inline int cpumask_subset(const struct cpumask *src1p,
>>  				 const struct cpumask *src2p)
> 
> 
>  
> Regards,
> Srivatsa S. Bhat



Thanks for all comments, I updated the patch according to your some suggestion.
Anyway, I don't mind if the patch is picked up. Maybe there is only one stupid 
guy in the world. :)
----------