New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Codebase lacks styleguide #499

Open
AaronOpfer opened this Issue Dec 9, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@AaronOpfer
Contributor

AaronOpfer commented Dec 9, 2016

This is something I'd like to have in a permanent fashion: a styleguide. Right now I see contradictory styles in the codebase. To the credit of the codebase, it's rarely inconsistent in the same file, but across files the style is a bit divergent.

I have no intention of starting a holy war here, I'm more interested in everything being the same than everything matching what I think. Sameness will make it easier for the codebase to be understood and contributed to. I've outlined some of my opinions below.


Tabs vs Spaces

It looks like we've chosen tabs for the most part already, but there are a few stragglers. I've also seen a few files that have mixed tabs and spaces, these should be fixed.

Open brace on same line or next line

I've flip-flopped on this topic a few times so I'm flexible either way. Looks like we tend to put opening braces on the same line. I'm cool with this.

Braces around single-line bodies

In my opinion this should be required.

// bad
if (cond)
    resp();

// good
if (cond) {
    resp();
}

Prevents errors when performing maintenance and makes diffs cleaner. It gets rid of the ability to do a single line if () x else y; but those should probably be written out on multiple lines or ternaries.

snake_case or camelCase

It looks like all of our first party classes are CapitalizedCamelCase which I support. But it looks like method names are all over the place. Brief example:

	bool              tty_enabled;
	bool              remove_stale_symbols;
	bool			  disableASLR;
	bool			  disableLazyBinding;
	QString           tty_command;

I don't have strong feelings on this topic. I have a slight preference for snake_case. In the past when I've worked on groups that have had disagreements about what style to go with, we went with the style that our biggest dependency used. In our case Qt seems to use camel case for everything.

I believe trailing underscores on private members is a common convention and it looks like we follow that convention pretty consistently. I did find a few places where this is not done, though, so might be worth a quick look.

Alignment

IMO we should try to avoid aligning tokens (like in the example above). It leads to strange diffs whenever anyone has to add a new element large enough to break the alignment. I find it a bit distracting as well. If we decide we want alignment (even though I don't like it) we should try to make sure we always use spaces for alignment, not tabs. Tabs between tokens will render differently on virtually every IDE and will never look right for everyone (see above example). Spaces are the only reliable way to do alignment.

No extra whitespace at the end of lines

This is pretty easy to prevent. Most editors support highlighting extra whitespace. Extra whitespace does nothing but take up space. A simple git ls-files | grep cpp | sed -i 's/\s*$//' cleans all cpp files immediately.


Regardless of what we ultimately come up with, we should convert the codebase to the chosen style in one commit once everyone's got their loose branches merged so that we don't wreck every outstanding branch. It would also be great to have a stylechecker run on a pull request.

@10110111

This comment has been minimized.

Show comment
Hide comment
@10110111

10110111 Dec 9, 2016

Contributor

I have to admit that much of the inconsistency in style is due to my carelessness. You may be able to confirm it using git blame :) .

Contributor

10110111 commented Dec 9, 2016

I have to admit that much of the inconsistency in style is due to my carelessness. You may be able to confirm it using git blame :) .

@AaronOpfer

This comment has been minimized.

Show comment
Hide comment
@AaronOpfer

AaronOpfer Dec 9, 2016

Contributor

It's cool. It really isn't a hindrance, I just want to be able to clean it up when I see it. Need a working styleguide to do that.

Contributor

AaronOpfer commented Dec 9, 2016

It's cool. It really isn't a hindrance, I just want to be able to clean it up when I see it. Need a working styleguide to do that.

@eteran

This comment has been minimized.

Show comment
Hide comment
@eteran

eteran Dec 9, 2016

Owner

@10110111, No worries :-)


I agree 100% that we need a style guide. The project has been around in some form or another for about 10 years, and my personal style has evolved over that time.

Here's what I usually try to do:

Tabs vs Spaces / Alignment

Indent with tabs, align with spaces. In other words a tab may be preceded by another tab or nothing. Any other white space should be spaces. If done correctly, this leads to consistent look across editors and diffs. And it's easy to get a more (or less) horizontally compressed look by adjusting tab settings.

Braces

I heavily prefer same line and I nearly always use braces.

if(something) {
    // whatever
} 

The only exception is if it is written as single line to emphasize that there are no braces:

if(something) func();

If it doesn't look good on one line, use braces.

snake_case or camelCase

I have done some flip flopping over the years. Here's where I'm at now preference wise:

void global_function();
void namespaced_function();
void memberFunction();
class ClassName;
namespace my_namespace;

I am currently having mixed feelings about using:

int private_member_var_;

and

int privateMemberVar_;

No extra whitespace at the end of lines

I try to eliminate it routinely, my editor of choice does some auto indentation that can result in lines which just have whitespace if I'm not paying attention.


Anyway, let's start putting something together in the Wiki with the understanding that the code isn't conformant yet. And once we settle on a nice, consistent style, we can refactor to match it as we go. LLVM has some reformatting tools which will get us most of the way there.

Owner

eteran commented Dec 9, 2016

@10110111, No worries :-)


I agree 100% that we need a style guide. The project has been around in some form or another for about 10 years, and my personal style has evolved over that time.

Here's what I usually try to do:

Tabs vs Spaces / Alignment

Indent with tabs, align with spaces. In other words a tab may be preceded by another tab or nothing. Any other white space should be spaces. If done correctly, this leads to consistent look across editors and diffs. And it's easy to get a more (or less) horizontally compressed look by adjusting tab settings.

Braces

I heavily prefer same line and I nearly always use braces.

if(something) {
    // whatever
} 

The only exception is if it is written as single line to emphasize that there are no braces:

if(something) func();

If it doesn't look good on one line, use braces.

snake_case or camelCase

I have done some flip flopping over the years. Here's where I'm at now preference wise:

void global_function();
void namespaced_function();
void memberFunction();
class ClassName;
namespace my_namespace;

I am currently having mixed feelings about using:

int private_member_var_;

and

int privateMemberVar_;

No extra whitespace at the end of lines

I try to eliminate it routinely, my editor of choice does some auto indentation that can result in lines which just have whitespace if I'm not paying attention.


Anyway, let's start putting something together in the Wiki with the understanding that the code isn't conformant yet. And once we settle on a nice, consistent style, we can refactor to match it as we go. LLVM has some reformatting tools which will get us most of the way there.

@eteran eteran added the code quality label Dec 9, 2016

@10110111

This comment has been minimized.

Show comment
Hide comment
@10110111

10110111 Dec 9, 2016

Contributor

If done correctly, this leads to consistent look across editors and diffs.

Maybe consistent, but not quite easy to use. E.g. you have to append ?ts=4 to GitHub diff URLs to have non-overwide diffs.

my editor of choice does some auto indentation that can result in lines which just have whitespace if I'm not paying attention.

Your editor might have an option to "cleanup on saving". I know this option from QtCreator. In Vim you can make an autocmd for this. Maybe some others also do have it.

Contributor

10110111 commented Dec 9, 2016

If done correctly, this leads to consistent look across editors and diffs.

Maybe consistent, but not quite easy to use. E.g. you have to append ?ts=4 to GitHub diff URLs to have non-overwide diffs.

my editor of choice does some auto indentation that can result in lines which just have whitespace if I'm not paying attention.

Your editor might have an option to "cleanup on saving". I know this option from QtCreator. In Vim you can make an autocmd for this. Maybe some others also do have it.

@eteran

This comment has been minimized.

Show comment
Hide comment
@eteran

eteran Dec 9, 2016

Owner

@10110111 that's a fair point, I hadn't considered the github diff factor. I'll have to think this one over to weigh the pros and cons.

Regarding my editor, well, I'm pretty sure it doesn't have such a feature as it is quite old. On the plus side, one of my other side projects is to rewrite it from the ground up using modern tools, at which point I can add that feature! LOL. I dunno how soon it'll be ready though ;-).

Owner

eteran commented Dec 9, 2016

@10110111 that's a fair point, I hadn't considered the github diff factor. I'll have to think this one over to weigh the pros and cons.

Regarding my editor, well, I'm pretty sure it doesn't have such a feature as it is quite old. On the plus side, one of my other side projects is to rewrite it from the ground up using modern tools, at which point I can add that feature! LOL. I dunno how soon it'll be ready though ;-).

@eteran

This comment has been minimized.

Show comment
Hide comment
@eteran

eteran Dec 9, 2016

Owner

I have started the beginnings of a style guide at: https://github.com/eteran/edb-debugger/wiki/Style-Guide

I will be slowly adding things to it as we discuss the different choices :-)

Owner

eteran commented Dec 9, 2016

I have started the beginnings of a style guide at: https://github.com/eteran/edb-debugger/wiki/Style-Guide

I will be slowly adding things to it as we discuss the different choices :-)

@AaronOpfer

This comment has been minimized.

Show comment
Hide comment
@AaronOpfer

AaronOpfer Dec 10, 2016

Contributor

I just realized we had not even yet covered naming conventions for local variables. There is a dizzying number of things to have opinions about...

Maybe it would be a bit easier to try to defer entirely to an existing style guide such as Google's styleguide?

Contributor

AaronOpfer commented Dec 10, 2016

I just realized we had not even yet covered naming conventions for local variables. There is a dizzying number of things to have opinions about...

Maybe it would be a bit easier to try to defer entirely to an existing style guide such as Google's styleguide?

@eteran

This comment has been minimized.

Show comment
Hide comment
@eteran

eteran Dec 10, 2016

Owner

Maybe, though while I agree with the vast majority of their guide, I do have some different preferences. Maybe we can just use it for inspiration ;-)

Owner

eteran commented Dec 10, 2016

Maybe, though while I agree with the vast majority of their guide, I do have some different preferences. Maybe we can just use it for inspiration ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment