Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1540270 > unrolled thread

[PATCH 1/1] Fixed to codestyle

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/1] Fixed to codestyle Ozgur Karatas <okaratas@member.fsf.org> - 2016-12-12 12:10 +0100
    Re: [PATCH 1/1] Fixed to codestyle Eric Sandeen <sandeen@sandeen.net> - 2016-12-12 14:50 +0100
      Re: [PATCH 1/1] Fixed to codestyle Joe Perches <joe@perches.com> - 2016-12-12 19:20 +0100
        Re: [PATCH 1/1] Fixed to codestyle Eric Sandeen <sandeen@sandeen.net> - 2016-12-12 19:40 +0100
          Re: [PATCH 1/1] Fixed to codestyle Ozgur Karatas <okaratas@member.fsf.org> - 2016-12-12 21:40 +0100
            Re: [PATCH 1/1] Fixed to codestyle Eric Sandeen <sandeen@sandeen.net> - 2016-12-12 21:50 +0100
              Re: [PATCH 1/1] Fixed to codestyle Ozgur Karatas <okaratas@member.fsf.org> - 2016-12-12 21:50 +0100
              Re: [PATCH 1/1] Fixed to codestyle Joe Perches <joe@perches.com> - 2016-12-12 22:00 +0100
                Re: [PATCH 1/1] Fixed to codestyle Ozgur Karatas <okaratas@member.fsf.org> - 2016-12-12 22:00 +0100
                Re: [PATCH 1/1] Fixed to codestyle Eric Sandeen <sandeen@sandeen.net> - 2016-12-12 22:20 +0100
        Re: [PATCH 1/1] Fixed to codestyle Ozgur Karatas <okaratas@member.fsf.org> - 2016-12-12 21:40 +0100

#1540270 — [PATCH 1/1] Fixed to codestyle

Hello,

I have error to use uuid and I think the functions should be used when -i'm eye-catching- "(* uuid)".
I tested it.

Regards,

Signed-off-by: Ozgur Karatas <okaratas@member.fsf.org>
---
diff --git a/fs/xfs/uuid.c b/fs/xfs/uuid.c
index b83f76b..cd8bc8e 100644
--- a/fs/xfs/uuid.c
+++ b/fs/xfs/uuid.c
@@ -33,7 +33,7 @@ typedef struct {
  * it just something that's needed for user-level file handles.
  */
 void
-uuid_getnodeuniq(uuid_t *uuid, int fsid [2])
+uuid_getnodeuniq(uuid_t (*uuid), int fsid [2])
 {
 	xfs_uu_t *uup = (xfs_uu_t *)uuid;
 
@@ -51,8 +51,8 @@ uuid_is_nil(uuid_t *uuid)
 	if (uuid == NULL)
 		return 0;
 	/* implied check of version number here... */
-	for (i = 0; i < sizeof *uuid; i++)
-		if (*cp++) return 0;	/* not nil */
+	for (i = 0; i < sizeof (*uuid); i++) 
+	if (*cp++) return 0;	/* not nil */
 	return 1;	/* is nil */
 }

-- 
2.1.4

[toc] | [next] | [standalone]

#1540332

On 12/12/16 4:53 AM, Ozgur Karatas wrote:
> 
> Hello,
> 
> I have error to use uuid and I think the functions should be used when -i'm eye-catching- "(* uuid)".
> I tested it.
> 
> Regards,
> 
> Signed-off-by: Ozgur Karatas <okaratas@member.fsf.org>

NAK

This doesn't fix code style at all; there is no need and no
precedence for i.e. (*uuid) in function arguments in the xfs code,
and you have broken indentation in the loop within the function.

Thanks,
-Eric

> ---
> diff --git a/fs/xfs/uuid.c b/fs/xfs/uuid.c
> index b83f76b..cd8bc8e 100644
> --- a/fs/xfs/uuid.c
> +++ b/fs/xfs/uuid.c
> @@ -33,7 +33,7 @@ typedef struct {
>   * it just something that's needed for user-level file handles.
>   */
>  void
> -uuid_getnodeuniq(uuid_t *uuid, int fsid [2])
> +uuid_getnodeuniq(uuid_t (*uuid), int fsid [2])
>  {
>  	xfs_uu_t *uup = (xfs_uu_t *)uuid;
>  
> @@ -51,8 +51,8 @@ uuid_is_nil(uuid_t *uuid)
>  	if (uuid == NULL)
>  		return 0;
>  	/* implied check of version number here... */
> -	for (i = 0; i < sizeof *uuid; i++)
> -		if (*cp++) return 0;	/* not nil */
> +	for (i = 0; i < sizeof (*uuid); i++) 
> +	if (*cp++) return 0;	/* not nil */
>  	return 1;	/* is nil */
>  }
> 

[toc] | [prev] | [next] | [standalone]

#1540505

On Mon, 2016-12-12 at 07:49 -0600, Eric Sandeen wrote:
> On 12/12/16 4:53 AM, Ozgur Karatas wrote:
> > 
> > Hello,
> > 
> > I have error to use uuid and I think the functions should be used when -i'm eye-catching- "(* uuid)".
> > I tested it.
> > 
> > Regards,
> > 
> > Signed-off-by: Ozgur Karatas <okaratas@member.fsf.org>
> 
> NAK
> 
> This doesn't fix code style at all; there is no need and no
> precedence for i.e. (*uuid) in function arguments in the xfs code,
> and you have broken indentation in the loop within the function.

Perhaps better would be to convert the xfs uuid_t typedef
to the include/uapi/linux/uuid.h appropriate struct and
maybe use a comparison to NULL_UUID_<type>

> > diff --git a/fs/xfs/uuid.c b/fs/xfs/uuid.c
[]
> > @@ -33,7 +33,7 @@ typedef struct {
> >   * it just something that's needed for user-level file handles.
> >   */
> >  void
> > -uuid_getnodeuniq(uuid_t *uuid, int fsid [2])
> > +uuid_getnodeuniq(uuid_t (*uuid), int fsid [2])

And to amplify Eric's comment:

that bit is confusing as it makes uuid look
like a function pointer.

> >  {
> >  	xfs_uu_t *uup = (xfs_uu_t *)uuid;
> >  
> > @@ -51,8 +51,8 @@ uuid_is_nil(uuid_t *uuid)
> >  	if (uuid == NULL)
> >  		return 0;
> >  	/* implied check of version number here... */
> > -	for (i = 0; i < sizeof *uuid; i++)
> > -		if (*cp++) return 0;	/* not nil */
> > +	for (i = 0; i < sizeof (*uuid); i++) 
> > +	if (*cp++) return 0;	/* not nil */

There shouldn't be a space after sizeof.

> >  	return 1;	/* is nil */
> >  }
> > 

[toc] | [prev] | [next] | [standalone]

#1540531

On 12/12/16 12:14 PM, Joe Perches wrote:
> On Mon, 2016-12-12 at 07:49 -0600, Eric Sandeen wrote:
>> On 12/12/16 4:53 AM, Ozgur Karatas wrote:
>>>
>>> Hello,
>>>
>>> I have error to use uuid and I think the functions should be used when -i'm eye-catching- "(* uuid)".
>>> I tested it.
>>>
>>> Regards,
>>>
>>> Signed-off-by: Ozgur Karatas <okaratas@member.fsf.org>
>>
>> NAK
>>
>> This doesn't fix code style at all; there is no need and no
>> precedence for i.e. (*uuid) in function arguments in the xfs code,
>> and you have broken indentation in the loop within the function.
> 
> Perhaps better would be to convert the xfs uuid_t typedef
> to the include/uapi/linux/uuid.h appropriate struct and
> maybe use a comparison to NULL_UUID_<type>
> 
>>> diff --git a/fs/xfs/uuid.c b/fs/xfs/uuid.c
> []
>>> @@ -33,7 +33,7 @@ typedef struct {
>>>   * it just something that's needed for user-level file handles.
>>>   */
>>>  void
>>> -uuid_getnodeuniq(uuid_t *uuid, int fsid [2])
>>> +uuid_getnodeuniq(uuid_t (*uuid), int fsid [2])
> 
> And to amplify Eric's comment:
> 
> that bit is confusing as it makes uuid look
> like a function pointer.
> 
>>>  {
>>>  	xfs_uu_t *uup = (xfs_uu_t *)uuid;
>>>  
>>> @@ -51,8 +51,8 @@ uuid_is_nil(uuid_t *uuid)
>>>  	if (uuid == NULL)
>>>  		return 0;
>>>  	/* implied check of version number here... */
>>> -	for (i = 0; i < sizeof *uuid; i++)
>>> -		if (*cp++) return 0;	/* not nil */
>>> +	for (i = 0; i < sizeof (*uuid); i++) 
>>> +	if (*cp++) return 0;	/* not nil */
> 
> There shouldn't be a space after sizeof.

and the "if" /should/ be indented under the for loop, because
it is within the loop...

I suppose simply:

-	for (i = 0; i < sizeof *uuid; i++)
+	for (i = 0; i < sizeof(*uuid); i++) 

would be fine on its own, though, because that is a bit
unusual/inconsistent.  I'll admit that I didn't spot
that change as I scanned over the unnecessary & incorrect parts
of the first patch. :)

thanks,
-Eric

>>>  	return 1;	/* is nil */
>>>  }
>>>
> 

[toc] | [prev] | [next] | [standalone]

#1540626

12.12.2016, 20:35, "Eric Sandeen" <sandeen@sandeen.net>:
> On 12/12/16 12:14 PM, Joe Perches wrote:
>>  On Mon, 2016-12-12 at 07:49 -0600, Eric Sandeen wrote:
>>>  On 12/12/16 4:53 AM, Ozgur Karatas wrote:
>>>>  Hello,
>>>>
>>>>  I have error to use uuid and I think the functions should be used when -i'm eye-catching- "(* uuid)".
>>>>  I tested it.
>>>>
>>>>  Regards,
>>>>
>>>>  Signed-off-by: Ozgur Karatas <okaratas@member.fsf.org>
>>>
>>>  NAK
>>>
>>>  This doesn't fix code style at all; there is no need and no
>>>  precedence for i.e. (*uuid) in function arguments in the xfs code,
>>>  and you have broken indentation in the loop within the function.
>>
>>  Perhaps better would be to convert the xfs uuid_t typedef
>>  to the include/uapi/linux/uuid.h appropriate struct and
>>  maybe use a comparison to NULL_UUID_<type>
>>
>>>>  diff --git a/fs/xfs/uuid.c b/fs/xfs/uuid.c
>>  []
>>>>  @@ -33,7 +33,7 @@ typedef struct {
>>>>    * it just something that's needed for user-level file handles.
>>>>    */
>>>>   void
>>>>  -uuid_getnodeuniq(uuid_t *uuid, int fsid [2])
>>>>  +uuid_getnodeuniq(uuid_t (*uuid), int fsid [2])
>>
>>  And to amplify Eric's comment:
>>
>>  that bit is confusing as it makes uuid look
>>  like a function pointer.
>>
>>>>   {
>>>>           xfs_uu_t *uup = (xfs_uu_t *)uuid;
>>>>
>>>>  @@ -51,8 +51,8 @@ uuid_is_nil(uuid_t *uuid)
>>>>           if (uuid == NULL)
>>>>                   return 0;
>>>>           /* implied check of version number here... */
>>>>  - for (i = 0; i < sizeof *uuid; i++)
>>>>  - if (*cp++) return 0; /* not nil */
>>>>  + for (i = 0; i < sizeof (*uuid); i++)
>>>>  + if (*cp++) return 0; /* not nil */
>>
>>  There shouldn't be a space after sizeof.
>
> and the "if" /should/ be indented under the for loop, because
> it is within the loop...
>
> I suppose simply:
>
> - for (i = 0; i < sizeof *uuid; i++)
> + for (i = 0; i < sizeof(*uuid); i++)
>
> would be fine on its own, though, because that is a bit
> unusual/inconsistent. I'll admit that I didn't spot
> that change as I scanned over the unnecessary & incorrect parts
> of the first patch. :)
>
> thanks,
> -Eric

Dear Eric;

Can you tell me the true code style? should use to (* uuid)? 
I'm learn to new and I'm newbies :)

Sorry,

Regards

Ozgur

[toc] | [prev] | [next] | [standalone]

#1540630

On 12/12/16 2:34 PM, Ozgur Karatas wrote:
> 12.12.2016, 20:35, "Eric Sandeen" <sandeen@sandeen.net>:
>> On 12/12/16 12:14 PM, Joe Perches wrote:

...

>>>>>   {
>>>>>           xfs_uu_t *uup = (xfs_uu_t *)uuid;
>>>>>
>>>>>  @@ -51,8 +51,8 @@ uuid_is_nil(uuid_t *uuid)
>>>>>           if (uuid == NULL)
>>>>>                   return 0;
>>>>>           /* implied check of version number here... */
>>>>>  - for (i = 0; i < sizeof *uuid; i++)
>>>>>  - if (*cp++) return 0; /* not nil */
>>>>>  + for (i = 0; i < sizeof (*uuid); i++)
>>>>>  + if (*cp++) return 0; /* not nil */
>>>
>>>  There shouldn't be a space after sizeof.
>>
>> and the "if" /should/ be indented under the for loop, because
>> it is within the loop...
>>
>> I suppose simply:
>>
>> - for (i = 0; i < sizeof *uuid; i++)
>> + for (i = 0; i < sizeof(*uuid); i++)
>>
>> would be fine on its own, though, because that is a bit
>> unusual/inconsistent. I'll admit that I didn't spot
>> that change as I scanned over the unnecessary & incorrect parts
>> of the first patch. :)
>>
>> thanks,
>> -Eric
> 
> Dear Eric;
> 
> Can you tell me the true code style? should use to (* uuid)? 
> I'm learn to new and I'm newbies :)

Well, rule #1 for newbies is "code style patches aren't
very useful, and usually are not welcomed by the project."

Making style changes just because checkpatch told you to is
not particularly helpful.  If it were important, it would have
been done by now.  If it hasn't been done by now, odds are
it's not important.  :)

If you are writing /new/ code, then sure, conform to the kernel
style, /aided/ by checkpatch.pl, and using your discretion as
well.

If you are just now looking at xfs/* code, best not to start
with "style" cleanups.  You'll find this to be true in general
across the kernel, maintainers are usually not thrilled to have
this kind of patch.

If you want to start with a new project, learn about the code,
learn what it /does/, learn how to use it.  use it.  Find things
that don't work as expected, or could work better.  Look into
bug reports and if you understand them, and the code involved,
try to write and test a fix.  But don't go looking for whitespace
nitpicks.

> Sorry,

No need to be sorry, this is how we learn.  ;)  But really, making
purely cosmetic changes for their own sake is not helpful in
general.

-Eric

> Regards
> 
> Ozgur
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [next] | [standalone]

#1540633

12.12.2016, 22:41, "Eric Sandeen" <sandeen@sandeen.net>:
> Well, rule #1 for newbies is "code style patches aren't
> very useful, and usually are not welcomed by the project."
>
> Making style changes just because checkpatch told you to is
> not particularly helpful. If it were important, it would have
> been done by now. If it hasn't been done by now, odds are
> it's not important. :)
>
> If you are writing /new/ code, then sure, conform to the kernel
> style, /aided/ by checkpatch.pl, and using your discretion as
> well.
>
> If you are just now looking at xfs/* code, best not to start
> with "style" cleanups. You'll find this to be true in general
> across the kernel, maintainers are usually not thrilled to have
> this kind of patch.

Dear Eric;
this information was very good and thank you, I will try for the better :)

> If you want to start with a new project, learn about the code,
> learn what it /does/, learn how to use it. use it. Find things
> that don't work as expected, or could work better. Look into
> bug reports and if you understand them, and the code involved,
> try to write and test a fix. But don't go looking for whitespace
> nitpicks.

I get it now, I understand but I think the error was only uuid functions.
Now, me more careful.

>>  Sorry,

>
> No need to be sorry, this is how we learn. ;) But really, making
> purely cosmetic changes for their own sake is not helpful in
> general.
>
> -Eric

I have mentioned above, thank you for all the information. 
You are helping me and  your mentoring in some way.

Regards

Ozgur Karatas

[toc] | [prev] | [next] | [standalone]

#1540640

On Mon, 2016-12-12 at 14:41 -0600, Eric Sandeen wrote:
> On 12/12/16 2:34 PM, Ozgur Karatas wrote:
[]
> > Can you tell me the true code style? should use to (* uuid)? 
> > I'm learn to new and I'm newbies :)
> 
> Well, rule #1 for newbies is "code style patches aren't
> very useful, and usually are not welcomed by the project."

I'd amend that to

"newbies should only use checkpatch on files in drivers/staging/"

and are generally welcome to do exactly that in order to learn
how to submit patches appropriately before finding something
actually useful to change in other areas of the kernel.

[toc] | [prev] | [next] | [standalone]

#1540642

12.12.2016, 22:50, "Joe Perches" <joe@perches.com>:
> On Mon, 2016-12-12 at 14:41 -0600, Eric Sandeen wrote:
>>  On 12/12/16 2:34 PM, Ozgur Karatas wrote:
>
> []
>>  > Can you tell me the true code style? should use to (* uuid)?
>>  > I'm learn to new and I'm newbies :)
>>
>>  Well, rule #1 for newbies is "code style patches aren't
>>  very useful, and usually are not welcomed by the project."
>
> I'd amend that to
>
> "newbies should only use checkpatch on files in drivers/staging/"
>
> and are generally welcome to do exactly that in order to learn
> how to submit patches appropriately before finding something
> actually useful to change in other areas of the kernel.

Dear Joe;
I understand, thank you very much but I use XFS and I'm trying to learn how to code to XFS filesystems. 
Maybe, I can try for XFS and I need to very learn more :)

Regards,

Ozgur Karatas

[toc] | [prev] | [next] | [standalone]

#1540672

On 12/12/16 2:50 PM, Joe Perches wrote:
> On Mon, 2016-12-12 at 14:41 -0600, Eric Sandeen wrote:
>> On 12/12/16 2:34 PM, Ozgur Karatas wrote:
> []
>>> Can you tell me the true code style? should use to (* uuid)? 
>>> I'm learn to new and I'm newbies :)
>>
>> Well, rule #1 for newbies is "code style patches aren't
>> very useful, and usually are not welcomed by the project."
> 
> I'd amend that to
> 
> "newbies should only use checkpatch on files in drivers/staging/"
> 
> and are generally welcome to do exactly that in order to learn
> how to submit patches appropriately before finding something
> actually useful to change in other areas of the kernel.

Ok, that's a fair amendment.  :)

-Eric

[toc] | [prev] | [next] | [standalone]

#1540624

12.12.2016, 20:14, "Joe Perches" <joe@perches.com>:
> On Mon, 2016-12-12 at 07:49 -0600, Eric Sandeen wrote:
>>  On 12/12/16 4:53 AM, Ozgur Karatas wrote:
>>  >
>>  > Hello,
>>  >
>>  > I have error to use uuid and I think the functions should be used when -i'm eye-catching- "(* uuid)".
>>  > I tested it.
>>  >
>>  > Regards,
>>  >
>>  > Signed-off-by: Ozgur Karatas <okaratas@member.fsf.org>
>>
>>  NAK
>>
>>  This doesn't fix code style at all; there is no need and no
>>  precedence for i.e. (*uuid) in function arguments in the xfs code,
>>  and you have broken indentation in the loop within the function.
>
> Perhaps better would be to convert the xfs uuid_t typedef
> to the include/uapi/linux/uuid.h appropriate struct and
> maybe use a comparison to NULL_UUID_<type>

Dear Joe;
Firstly, I have studied and so I thought it was correct to use it as (* uuid) in this regard Mr. Eric corrected me and he is explanatory to me.

fs/xfs/uuid.c:21: WARNING: do not add new typedefs
fs/xfs/uuid.c:36: ERROR: space prohibited before open square bracket '['
fs/xfs/uuid.c:54: WARNING: sizeof *uuid should be sizeof(*uuid)
fs/xfs/uuid.c:55: ERROR: trailing statements should be on next line
total: 2 errors, 2 warnings, 63 lines checked

>>  > diff --git a/fs/xfs/uuid.c b/fs/xfs/uuid.c
>
>
>>  > @@ -33,7 +33,7 @@ typedef struct {
>>  > * it just something that's needed for user-level file handles.
>>  > */
>>  > void
>>  > -uuid_getnodeuniq(uuid_t *uuid, int fsid [2])
>>  > +uuid_getnodeuniq(uuid_t (*uuid), int fsid [2])
>
> And to amplify Eric's comment:
>
> that bit is confusing as it makes uuid look
> like a function pointer.
>
>>  > {
>>  > xfs_uu_t *uup = (xfs_uu_t *)uuid;
>>  >
>>  > @@ -51,8 +51,8 @@ uuid_is_nil(uuid_t *uuid)
>>  > if (uuid == NULL)
>>  > return 0;
>>  > /* implied check of version number here... */
>>  > - for (i = 0; i < sizeof *uuid; i++)
>>  > - if (*cp++) return 0; /* not nil */
>>  > + for (i = 0; i < sizeof (*uuid); i++)
>>  > + if (*cp++) return 0; /* not nil */
>
> There shouldn't be a space after sizeof.

You can see below:
fs/xfs/uuid.c:54: WARNING: sizeof *uuid should be sizeof(*uuid)
Regards,

Ozgur

>>  > return 1; /* is nil */
>>  > }
>>  >

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web