Coding style

[Horrowind]

trafficstars

Since we have a different coding style, I would like to know what your precise coding style is, so that I can create a clang-format file and forget about this issue.

If you want, we can add this clang-format file to the repository. We could also reformat all files with it, to achieve some consistency. I tried this before, it made some of the files more readable, but this is arguable in the more macro heavy files like arch-65816.cpp.

I also think that there are some ways with CI to automatically reformat pull requests, but this is an issue for @randomdude999. I would disfavor if CI would just reject pull requests which have not been run through clang-format, since this just increases the burden on new developers.

[trillllian]

It seems that for most of the codebase, indents are 1 tab and braces always go on their own line. Pointers chaotically have the * near the type, near the name or with spaces on both sides (i think i once even saw a pointer with spaces on neither side). I know that the most logical would be to have them near the name, but it looks a bit unintuitive.

I'm pretty sure it's possible for Travis CI to add a commit to the pull request that fixes the formatting automatically, I'll look into it right now since I have never used that feature before.

[RPGHacker]

I honestly didn't think too much about coding style when starting to do work on Asar. I'm not even sure if original Asar code had any consitent coding style at all. I also don't care about it too much for the current Asar version (would only really care if I rewrote it from scratch). That being said, my personal preferences (as far as Asar is concerned) are:

• brackets go on their own line
• use tabs as indentation (unless when formatting a table, graphic or something like that)
• UNIX line endings (when VS doesn't silently sneak some Windows line ending in there)
• type* varialbe for pointers, rather than type * variable or type *variable. The first variation is the only one that makes sense to me.
• Asar uses a lot lowercaseinasingleword names, which I don't like since it's quite unreadable. I prefer lower_case_with_underscores where possible. (If I were rewriting Asar from scratch, though, I would use CamelCase and lowerCamelCase).
• I don't like abbreviations. Unless they're for something obvious and commonly used (such as indices, where i, j, k etc. are okay), I avoid them whereever possible. Basically, names of variables and functions should speak for themselves as much as possible. This means, for example, that I would call a function get_next_character_in_string() rather than getchr()or getnext() or whatever. I don't mind variable or function names getting ridiculously long, because it really doesn't make typing any more inconvenient for me (and it's what I use auto-completion for, anyways). It also makes functions easier to find for people not familiar with the code-base (when you're looking for string-related functions, it's easier to find functions that contain the word "string" than to find functions only containing "str").

That's about all I can think of.

[trillllian]

A limitation I've just realized is that Travis can't push to other people's repositories, so the best we could do is automatically fix the formatting right after the pull request is accepted.

Also, AFAIK Git handles line endings automatically (on Windows they are always \r\n, on Linux they are \n).

[trillllian]

All right, I'm working on a clang-format file right now, what should the column limit be? The default is 80 but I feel that with long variable names or just long lines in general that will be a little too narrow, I'd go for 100 or 120.

[RPGHacker]

What exactly does the column limit do? Split function calls and stuff into multiple lines when they get too long?

[trillllian]

Yes. Also splits function declarations, really long strings, i think preprocessor defines too. Basically, unless you have 1 name that's over <limit> characters long, you'll never have a line longer than <limit>.

[RPGHacker]

I see. I have no strong opinion on that then. I tend to use automatic line wrap or just wrap my lines by intuition. Either setting is fine with me.

[trillllian]

Okay, I wrote a format file (choosing what I believe to be sensible defaults for all unspecified things) and ran clang-format on the entire src/ directory. Looks like it will need a bit more tweaking (currently it causes a syntax error in the python binding), but you'll get an idea of what the code would look like. You should also take a look at the .clang-format file in the zip and see if you agree with all the settings there (here's what each option does). clang-fmt-test.zip

[RPGHacker]

I barely see a difference, to be honest. It seems to have left most of the code untouched. For example, I still see two different methods of placing brackets everywhere in the code. Don't quite understand how it's functioning.

[trillllian]

The only two ways of placing brackets that I allowed are either brackets on separate lines or if the block inside the brackets in only 1 line then the brackets are in the same line as the condition and the code inside them (i.e. if (a) { return; }). (also it might be related to how I picked some settings based on how the current Asar codebase does it)

[RPGHacker]

Doesn't quite seem to do that, though. For example, I'm also seeing this:

static enum {
	ratsmeta_ban,
	ratsmeta_allow,
	ratsmeta_used,
} ratsmetastate = ratsmeta_ban;

or this

inline void write1_65816(unsigned int num)
{
	verifysnespos();
	if (pass == 2) {
		int pcpos = snestopc(realsnespos & 0xFFFFFF);
		if (pcpos < 0) {
			movinglabelspossible = true;
			error(2, S "SNES address $" + hex6((unsigned int)realsnespos) + " doesn't map to ROM");
		}
		writeromdata_byte(pcpos, (unsigned char)num);
		if (pcpos >= romlen) romlen = pcpos + 1;
	}
	step(1);
	ratsmetastate = ratsmeta_ban;
}

It doesn't seem to know what it wants to do. :/

[trillllian]

Okay, this one seems to work better: I disabled all options for placing braces on the same line. It's more consistent now, even though it takes more space. clang-fmt-test.zip

Also that ratsmetastate line is very weird. Intellisense keeps saying there's an error, but it compiles fine. I assume clang-format can't figure it out either.

[RPGHacker]

Don't really mind it taking more space (I'm basically used to it and tend to code like that myself).

Yeah, it has changed more stuff this time, but it's still kinda weird and inconsistent. For example: it still uses a different bracket style for the enum. But whatever, I suppose there's simply stuff it can't do. It's still probably better than it was without any reformatting.

[trillllian]

That (ratsmetastate) is literally the only enum that didn't get formatted properly (there's also one enum with enum \n { \n ... \n } init; but i wouldn't consider that incorrect since putting the name on a separate line is even more confusing). Every other opening/closing bracket that isn't on its own line is either the closing bracket of a do/while loop or array initialization.

[RPGHacker]

Ah, I didn't realise you were talking about the enum before. Weird that it can't deal with it. Maybe it can't handled C-styled enum declarations or something.

[trillllian]

All right, I think I have a setup for the automatic code style thing - see here, mainly .travis.yml and deploy.sh

[trillllian]

@Horrowind @RPGHacker do you think the clang-format file i posted earlier is good? I'd use it and start testing the auto-formatting on my fork now.

A few limitations that I realized while working on it:

• Travis can't push to someone else's repository (even if the repo is a fork of one that travis has access to), so the best we could do is fix the formatting right after the pull request gets merged.
• Also, currently the formatting fix is not applied when the build is failing (i.e. the code doesn't compile or the test suite fails), and I don't think it's possible for me to make it so when the code compiles fine but fails tests then the formatting is applied.
• Not a real limitation but since there's a new commit right after a push, every time you do a push you need to remember to do a pull a few minutes later (after travis has commited the format fix). Travis doesn't create a commit if the formatting is already fine though, so you could just run clang-format locally before committing.

[RPGHacker]

Seems fine to me. I don't absolutely need the reformatting, to be honest, I can work without it, but if you think the gains from it are worth it and if it doesn't cause any actual inconveniences, I don't mind.

[Horrowind]

To be clear, I only started the discussion because I liked to create/have a style file which I can use check if I messed up formatting somewhere. Opening braces on new lines bite me regularly :). Thanks for creating the file, @randomdude999! I just mentioned the possibility of using this with CI, because I have seen this somewher, not knowing what this can achieve. I also don't really need this with CI, I think, I can run clang-format on my machine.