Style compliancy problems

hausdorf commented 13 years ago

Structs

Previously, I thought it was the case that typedef'ing structs was mainly for readability. Recent readings indicate that this is incorrect, and libgit in general seems to agree.

We need to:

Remove typedefs in structs that are not opaque.
For structs that are opaque, we want to access them only via functions that are designed to handle them.
Error Codes

We often return -1 after an error problem; we should return error codes instead. Per @crakdmirror's request, these error codes seem to be defined in include/common.h. Not sure if that's all of them.

Function declarations

Should we declare functions? Where? What sort (e.g., static)?

Line wrap

Probably 80 columns.

Tabs vs Spaces

libgit2 uses tabs. End of story. Spaces are unacceptable. Additionally, it is not true that any instance of 4 spaces should become a tab. Consider the following:


   int some_func(int param,
                 int param2)

Note that the initial indentation for line 1 and 2 are the same -- we use 1 tab. BUT, the second line must use spaces after this 1-tab indentation so that the params line up.

Another example is here; we indent both the first and the second line with 1 tab, and then use spaces to line up the xdl_change_compact() calls. There are more than 4 spaces.

Comments

This is C99, so either a // Comment or `/* Comment */ format are acceptable.

Spaces

for(i=0; i<len; i++)
if(...)

Should be...

for (i = 0; i < len; i++)
if (...)

Notice: spaces after for, if, else, else if and between the following symbols:

=  +  -  <  >  *  /  %  |  &  ^  <=  >=  ==  !=  ?  :

Braces

Braces are placed on the same lines in control-flow statements, e.g., conditionals, loops, and switches. The are NOT placed on the same lines of functions. See style guide for more details.

YES:

int has_cow(struct farmer *bob)
{
    if(bob->cow) {
        return 0;
    }
    else {
        return NO_COW;
    }
}

NO:

int has_cow(struct farmer *bob) {
    if(bob->cow)
    {
        return 0;
    }
    else
    {
        return NO_COW;
    }
}

basepi commented 13 years ago

Are the error codes all located in one location? And is there any documentation on their meaning? Because sometimes they can be pretty cryptic.

hausdorf commented 13 years ago

include/common.h seems to be the place.

Not sure if that's all of them.

(In case you were wondering, I found this by looking for a place where an error occurs and grep'd for it.)

basepi commented 13 years ago

Fantastic, just what I was looking for. Thanks.

On Thu, Apr 21, 2011 at 9:06 PM, DrSleep < reply@reply.github.com>wrote:

include/common.h seems to be the place.

Not sure if that's all of them.

Reply to this email directly or view it on GitHub: https://github.com/crakdmirror/libgit2/issues/22#comment_1042519

trane commented 13 years ago

for(i=0; i<len; i++)
if(...)

Should be...

for (i = 0; i < len; i++)
if (...)

Notice: spaces after for, if, else, else if and between the following symbols:

=  +  -  <  >  *  /  %  |  &  ^  <=  >=  ==  !=  ?  :

hausdorf commented 13 years ago

Note, braces go on the next line for functions, and stay on the same line for conditionals. DO NOT put braces on the same line for functions. See "Braces" section above.

basepi / libgit2