Code quality

[archshift]

Is there any enforcement of a style guide for this project?

• Parts of code are all over the place.
• There are tons of random extern and static members and functions that belong to no class.
• You redefine BOOL for apparently no reason (?), as well as TRUE and FALSE (?).
• C code is littered among supposedly C++ files, and from what I've seen the C standard library is used more often than the C++ counterpart.
  • Tons of C strings all over the place as opposed to std::string
  • Where C strings aren't used, wxStrings are, which will make it very, very difficult should you ever decide to move away from Wx.
  • Basically no use of C++11
• Main directory is cluttered with MSVC solution files and build scripts (also, do you really still need to support VS 2010?).
• You have no CONTRIBUTING.md (!!).
• Incorrect alignment all over the place

[gigaherz]

We have a few guidelines... But the code has been programmed over many years, by various people with different styles, and although we form a team, we don't have such strict rules or enforcement. If the code looks good, it's good enough for us.

We use "C/C++", not specifically C++. PCSX2 was originally C, and we converted some parts of the code to C++, but only when it was an advantage, and not a waste of time. We don't use C++11 because most of the codebase was written long before C++11 was finalized, and we aren't going to waste time rewriting existing code just because C++11 has some fancy feature.

No comment on the BOOL part.

What do you mean with incorrect alignment? Oh you mean code formatting. The github viewer is using 8-character tabs, while we assume 4, so chances are it's not that badly aligned as you think.

[archshift]

The thing is, is that "good enough for us" makes it very difficult for new contributors to get into this project, and is probably a major reason why PCSX2 is suffering from a lack of contributors. When people can't make heads or tails of the arrangement and flow of the program, you begin to see loss of interest.

[gigaherz]

There's nothing we can do at this point, because as we keep saying over and over, we don't have the time for big projects anymore. Also, that hasn't stopped people in the past. It didn't stop a group of people from forking pcsx2 to work on new experimental features, which later got merged into the main pcsx2, and those people became part of the team.

We have multiple ways people can contact us: The issue tracker, forums, IRC channels, even email. We are not a big project, people-wise. We don't have a strict review policy, or a method to follow in turn to contribute because we don't believe one is necessary. People are free to jump in and make a PR with their changes. If they are scared someone may already be working on it, they can ask. If they want to tell others they will be working on it, they can make a PR in advance, with an explanation about what they are working on. So anyone is welcome to help. Fork the repo, make a branch, start coding, send a PR.

[ameenross]

Something as simple as mixing tabs and spaces for indenting can be fixed with a simple bash command in bulk. So not all issues in that list are complicated to work out IMO

[xsacha]

Code clean ups don't take very long. In fact, if you came up with some rules for this project you'll find contributors jumping in just to apply those rules and make the code easier to read. I'd change it now but I'm not sure what you guys even want. You say you guys use 4 character tabs -- like most projects. That's not something github has a problem with. In fact, if you display it with 16 character tabs, it should still look exactly the same. The problem would be from inter-combining spaces and tabs for indentation or not using spaces to align. You see, I'm not even sure if you guys do use 4 character or 2 character or 8 character or spaces or tabs -- they are all used in this project.

As for C++11, it comes with your compiler. It's there and it's beneficial. It makes your code many many times more readable, especially in the case where it was an unreadable mess beforehand. I can pick just about any file in the repo and it would look more readable with C++11. Some things that are done in 100 lines of C could be done in a few lines of C++11. You reimplement a lot of things that are now standard. Not just once, but once per file. Many times they are implemented in ways that are buggy, not consistent and require deep thought to understand the meaning behind them. For instance when reading a line that has '... = ((ff0 >> (3*0+12)) & 7) - 1;' and wondering what the logic is behind it.

[archshift]

Some questions that need answering (and enforcement as a style guide, not just whimpy guidelines):

• Tabs or spaces? Tab width?
• Curly brackets on the same line or on a new line?
• Spaces between (e.g.) if and the condition or not?
• Commenting style and frequency (doxygen?)
• Magic numbers? (Constants that are not thoroughly explained)
• Classless fields and functions (with reasonable exceptions)?
• NULL vs std::nullptr?
• C vs C++ style type casts?
• Bare vs smart pointers (with reasonable exceptions)?
• One class per file (with reasonable exceptions, e.g. Lots of tiny classes)?
• C vs std strings?
• Variable prefixes?
• Variable/function/class case styling?
• etc.

A standard, at least, must be created, at least as something to strive to. It's very difficult to maintain a project when you can't read the code.

[sudonim1]

Yeah um, whitespace needs to be standardised. Not even all regularly contributors agree on this. We tried to agree on curly brackets on new lines, it's not enforced everywhere though. Keywords that aren't separated by whitespace look strange. Comments are good. Automated documentation generation creates a lot of busywork imo but I'm not speaking for the team on this one. Unexplained magic numbers are bad, duh. Everything does not need to be a class. Some things should be that currently aren't though. std::nullptr is C++11, definitely not. All types of casts have their place. I guess you're thinking the new C++11 smart pointers? Just no. One class per file is awful Java nonsense. C strings have their place. Okay, naming is something we actually should have standards for.

[archshift]

What's wrong with C++11? Are some of your contributors stuck with MSVC 2008?

[sudonim1]

Many C++11 features are pretty disgusting.

[archshift]

Well, just because many of the features are 'disgusting' as you put it, does not mean that there aren't many others which are very useful and even necessary at this point in time. The fact that a feature comes with the C++11 standard should not be enough for you to immediately discount its value.

[ameenross]

Many != all

[sudonim1]

I certainly think we don't want to mandate using C++11 features without discussion.

[gigaherz]

There are certain C++11 features I'd favor, like using range-based loops where enumerating STL containers, or using "auto" (compile-time type inference) on long (templated) variable types.

But I don't believe REQUIRING any of those features is good, specially when a lot of people dislike the new skin in vs2012/2013, and would like to keep using vs2010. (I know other people prefer vs2010 because they want to keep coding in XP, but I have no pity for them)

[xsacha]

msvc2010 has plenty of C++11 support (not all, but enough). You could go with what is supported in that release. Of course no one is going to use C++11 features for the sake of it. I don't think any project does that and it doesn't really make sense to assume that's what supporting C++11 means. Only if they actually help readability or remove a situation where pcsx2 reinvents the wheel.

I don't really understand the reason for needing to discuss its usage at all. It comes with the compiler that everyone is using, it's built-in (no extra headers), it makes the code more readable, it's easier to code (less lines, more features). If there's some feature from C++11 that you are already using but your own frankenstein implementation, why not use the C++11 version? If there's some feature from C++11 that works around using hundreds of lines of unreadable code, adds no bloat to the project, no extra lines, makes it hugely readable, why not use the C++11 version?

[archshift]

I also wouldn't have any pity for developers refusing to update their compilers (to a drastically improved version) because, in the case of Visual Studio, it also comes with an IDE redesign.

[archshift]

Using features supplied by the C++11 standard is not 'forcing' anything on anyone. It's merely taking advantage of useful features that are available to you.

Developers have plenty of options when they're using an outdated and obsolete Visual Studio. They can update their IDE. They can install mingw. They can dual-boot or use a Linux VM.

In a world of quickly-evolving technology, developers have to evolve quickly as well.

[gigaherz]

Yes but using rude words: we don't have to fuck everyone over just because someone wants the code to be more "pretty".

[archshift]

Except you wouldn't be fucking anyone over, because everybody is capable of adapting to such a change.

Regardless, this isn't even about the code being more "pretty". You are suffering from a lack of contributors because people can't understand your code. Not only is it a mess, but there is no effort to try and fix it. Using C++11 features where they are called for is just one tool to try and tame the codebase of PCSX2.

[sudonim1]

C++11 features won't make the code more approachable, it'll make it incomprehensible to everyone but the few people who have actually studied the new standard.

[xsacha]

The current code is incomprehensible to people who know C/++ inside and out. C++11 is fairly easy to understand, simply out of the fact that the syntax is cleaner, less code and simpler. It certainly would make it much more approachable.

[sudonim1]

That's a problem of convoluted logic, nothing that can be taken care of with a few shiny new toys.

[pauldacheez]

FYI, anyone who starts learning C++ today learns the C++11 stuff along with or in place of previous parts of the language – older code that can be C++11-ified but hasn't can end up being incomprehensible to them. And I say this from experience!

[xsacha]

The logic is fine. It's the implementation, borne from a time when you had to do everything yourself or throw in an external dependency (which may have to be maintained or have changing APIs). What you see is a lot of bare C code, void of any templating, classes, lists or anything else that would aid readability, maintenance and bug-squashing. C++11 is just another step above improving the code with C++. May as well do both at once.

[sudonim1]

No, it's the logic. You just don't know the code well enough to understand how truly twisted some parts are.

[archshift]

Both are issues, and both should be fixed. One more major issue is documentation, which is truly lacking throughout this project.

[gregory38]

Let's stop trolling. C++ remains C++, whatever version you add it. C++11 is recent and not all latest compiler support it properly and entirely. For sure more and more C++11 feature will appears on the code-base but it won't magically reduce complexity of the program. You can do a lots without C++11, some people even write a PS2 emulator with it ;) So what are the 3 top features that will make the code better?

Please read the QA chapter of the wiki. Initial goal is to make PCSX2 compatible with clang so static tool can be used.

Why remove support of not so old compiler (seriously 4 years, in this case we can drop win7 support too) just because std::null_ptr is prettier than NULL? Don't you think it will be better to improve first all parts that will work for everybody?

[archshift]

Let's stop trolling.

Yes, because worrying about the longevity of this project is, in fact, trolling.

Please read the QA chapter of the wiki.

Trust me, I have. In fact, I was the one who last edited it, because it was a mess before.

Discuss your future contribution with us before coding it

Let's avoid duplicate work! Besides, the specification could be clarified this way.

This is what we're doing right now. The specification we're clarifying about contributing doesn't exactly exist, which is exactly why we're discussing.

Why remove support of not so old compiler

Because the age of VS's shitty compiler doesn't matter, what matters is the implementation of the standard. Microsoft likes to take their sweet time doing so, and only releases a new version once a year. That's the reason why these compilers aren't that old, but still obsolete.

in this case we can drop win7 support too

That's ridiculous. If you haven't noticed, single versions of developer software have a lower lifespan than entire OSs. Furthermore, the newest versions of MSVC create programs that can be run on older versions of Windows.

Don't you think it will be better to improve first all parts that will work for everybody?

The thing is, is that we don't have to do anything 'first'. All I'm asking in this issue is for the creation (and enforcement) of a standard so that anyone can try and help improve the quality of this codebase.

[angstsmurf]

Xsacha, you seem to know what you're talking about, but it is a little bit vague. Do you have a concrete example of code that could be simplified or eliminated by using C++11 features?

[archshift]

Xsasha question repository. I guess not.

[gigaherz]

Uh ... that's bullshit. Please find somewhere else to spread FUD, we have little enough time as it is, we don't need to be constantly wiping the shit from the floor.

[archshift]

Sorry, bringing that up really was uncalled for, but I'd still like to know your opinion about these issues.

[gregory38]

Don't worry project is more than 10 years old it will survive new C++ standard. The trolling is about the C++11 as minimal requirement to code. So let's get it straight. C++11 is allowed if it worked on GCC4.6+ and MS 2010. No if you have a very good reason to use a C++11 feature for a specific code location, it could be allowed with some ifdef (for example someone is implementing std::thread).

On the coding rules, I will give you my personal opinion. Enforcing rules never works. There is always exception beside nobody on this planet gets the same definition of nice code. So let's do it easy, feel free to do whatever you want that made the code easier to read but try to follow the current approximative standard of the project (tab = 4 white space, debugger friendly a statement by line). White space, curly bracket is a religious matter. Ultimately I would push enough to use an auto-format tool like astyle. And all those discussions will go to oblivion. Same for C++11, clang will take care of null_ptr, auto range loop. And that it's.

MS 2010 will be dropped someday for sure. Patience is a scarce virtue :)

[archshift]

Don't worry project is more than 10 years old it will survive new C++ standard

Why is the age relevant? I don't doubt it'll 'survive' a new standard, but refusal to allow the use of a new standard is something else.

No if you have a very good reason to use a C++11 feature for a specific code location, it could be allowed with some ifdef (for example someone is implementing std::thread).

This is exactly how you get spaghetti code and exacerbate the issue.

Enforcing rules never works

This is so untrue. Enforcing contributor rules works for, let's see, Dolphin, Linux, and, well, pretty much every single organized open-source project.

Ultimately I would push enough to use an auto-format tool like style.

I agree 100% with you on that, but simply using the tool isn't enough. Rules need to be made (and again, enforced) preventing developers from screwing with the style in their PRs and commits.

And all those discussions will go to oblivion. Same for C++11, clang will take care of null_ptr, auto range loop. And that it's.

I don't quite understand what you're trying to say here.

---

Again, rules only about whitespace and cosmetic formatting aren't enough (although that's more than what currently exists). #ifdefs for supported compiler features are a horrible solution for a horrible problem.

[archshift]

@angstsmurf There's your example. A reimplementation of std::thread.

[gigaherz]

WE ALLOW C++ FEATURES. WE DON'T HAVE ANY RULE AGAINST C++11.

As long as the code compiles in vs2010+ and whatever gcc is commonly used these days, it's ok for us. All we care is that the new code is readable. The old code we'll improve it over time, as we see fit. Unless someone makes small incremental updates to it, with clear improvements that we can agree are worth merging.

You know what? If you want a style guide with clearly enforceable rules, write one. We'll review, suggest amendments, and once we can agree about all those rules, then we may begin to enforce it for NEW code. Feel free to open a google doc, a GIST, a piratepad, or whatever your favorite "multiplayer notepad" may be. Don't just complain, make it happen.

[ramapcsx2]

I feel a lot of the things asked for are creating busywork that will needlessly tie up our resources.

If a feature you know of can improve genuinely unreadable code then please provide an example and we can decide if we want to adopt it. We will probably do so!

An overall plan with strict rules is nice for new / small / overstaffed projects but for us it is currently unreasonable. We would have to fix all the listed problems and then take great care that everyone always works by these new rules in the future (until something new comes around and this game starts again). I feel that this is too much to ask for our large code base and with our small team, and also I doubt that serious contributors will just walk away scared, just because a particular module was hard to read.

Please acknowledge that PCSX2 as a project always worked with the code being of somewhat mixed quality (It was very much worse once, believe me). The way it got better was by people rewriting questionable parts, hoping to get better compatibility by removing hacks and applying newly gained knowledge. Sometimes that introduced new oddities (microVU) but the benefit of the work overall was much bigger than these details.

Well, that's some thoughts of mine on the topic.

[archshift]

Write one. We'll review, suggest amendments, and once we can agree about all those rules, then we may begin to enforce it for NEW code. Feel free to open a google doc, a GIST, a piratepad, or whatever your favorite "multiplayer notepad" may be. Don't just complain, make it happen.

Great!

[pauldacheez]

Serious suggestion: why not just lazily adopt Dolphin’s coding standards? It’ll avoid 90% of the bikeshedding (since they’ve already done it), plus @Lioncash and other Dolphin devs who work on pcsx2 won’t mix the two standards up.

[sudonim1]

Well I don't like everything in Dolphin's standard, can't speak for other developers, but it's a saner starting point than the open pull request from @archshift. That said...

Here's the thing about this whole drive: it seems like an excuse to waste code reviewing time on trivial changes in the name of uniformity rather than readability, a concession to OCD and a way to seem helpful while actually just poking things meaninglessly.

[pauldacheez]

Consistency and uniformity are rather major parts of readability. And not fixing the OCD things is a great way to distract from the actual code – I mean, if you're repeatedly noticing stylistic issues, you’re probably not noticing real issues, are you? Developers don’t just sit there and masturbate to clean code, they work with it.

[gigaherz]

I suppose my OCD isn't as strong, or maybe I can simply cope better with multiple coding styles, but I only really apply consistency within files or modules (closely related code that exists within one or more files). I don't really mind it if the interpreters are written in one code style, and the recompilers in a different one. So long as there is a consistency within the related code.

So far in this project we have had the implicit policy that everyone writes the way they enjoy the most. I do not recall the team ever having any fight about coding style. We all cared more about having the emulator work, than how the code looked like. (And really I want to emphasize that the code now is quite a lot more readable and organized than it was 10 years ago, although it's also considerably larger).

There's certain style choices where our preferences really don't match, and we know it. Most of them, we learned to ignore or accept them. Possibly because so far we have always valued the effort more than the looks.

That said, there's some style choices that I really can't agree with, and although other developers may prefer them and make use of those styles while writing, and I may be able to ignore that, I most probably wouldn't be able to remain part of a project if those rules were enforced. A few of those rules, are present in Dolphin's style guide.

So far, as I am reading it: All variables should be lowercase with underscores separating the individual words in the name.

I didn't have any intention to contribute to that project to begin with, but after reading this, I will probably think it twice before I'd ever choose to do so. The only place I can accept underscores used as word separators (instead of casing), is in all-uppercase names. I do also happen to prefer NOT using all-uppercase for constants, I prefer to reserve that for preprocessor definitions and macros, so that I can easily distinguish them from functions and variable names. But it doesn't hit me as strongly as seeing lowercase with underscores.

Class layout should be in the order, public, protected, and then private.

This one, is even worse. Class fields go at the top. Always. No exceptions. Whenever I see a class with fields at the end, I either close the code, or re-arrange it. If I don't have the liberty of rearranging every single code file (because it's specified in the rules), then I'd probably just walk away, and forget I ever considered working with that code. Yes, I'd quit a job that enforced this, unless it paid a whole lot more money than I'm used to.

The preferred form of the increment and decrement operator in for-loops is prefix-form (e.g. ++var).

I have a strong dislike for prefix increment/decrement, anywhere. I can "accept" it, but if I'm working with code using prefix notation, and I have the option, I'll probably change them to suffix sooner or later. If I was to contribute in a project, this would be an extra negative mark which may help tilt the balances.

Luckily, Dolphin does not have the one rule that I loathe the most: "always write the constant first in a comparison (2 == something)". That's a "nothing to do here" situation. I wouldn't even think twice.

Again, I may be better than others at understanding code. I may simply have a weaker OCD (I may care how things are arranged in one room, but I don't mind if things are arranged differently in the next one -- they are different rooms). And while I'd like to make things more attractive for new contributors, I really don't want to scare any of them away just because some people thought the code would look nicer if it was written in the wrong style.

Sorry for the rant, I just wanted to make my feelings clear.

[sudonim1]

Ultimately what this comes down to is it being too late in the project's lifetime to impose a single standard for almost anything on the code. I'm okay with setting certain loose guidelines, but not strict rules that would necessitate needlessly poking every file in the tree and upsetting everyone who already works on the project.

[xsacha]

@gigaherz I also don't have OCD. Having more than one style in a single set of files is very disturbing to work with and read though. I don't mind if all the JIT files have one style and all the Common files have another style (it's even done this way in PPSSPP as it does actually make sense for JIT to have a different style). The point is if you have random tabs in a file that is mostly spaces and, for example, some variables are prefixed with , some are prefixed with m and then some have a suffix, it gets really annoying and hard to read.

Some projects do one style for everything, some do one style for a set of files. The important element is that there is some guideline for how to commit the code so that people don't just willy-nilly stick random code everywhere which makes reading, diffing and contributing extremely hard.

As for not liking the -- prefix for decrementing in for loops, http://hastebin.com/axavizivon.js There's over 50 occurrences in pcsx2 (excluding the 50 or so in wxwidgets). Hundreds more if you include while loops or if statements. Even more if you check ++. Of course there's just as many times when it isn't used. The same with everything else in the Dolphin style guide. It is used -- and not used -- in pcsx2. A big mish-mash.

There are parts of the styling that are much more important. Worrying about prefixes and suffixes is really at the pointy end of styling discussions. There are much serious issues here with redundant coding, unnecessary defines and typedefs (pet peeve of the day, pcsx2defs among others). Redefining things as simple as isdigit a few times, reimplementing everything from the STL.

Oh I can comment again :) I thought I wasn't allowed to comment before.

[gregory38]

There's over 50 occurrences in pcsx2

Well 101-63, give 38 (last time I checked it was below 50 :) ). And some lines are duplicated because we have 2 versions of zzogl (zzogl-pg-cg is pure legacy for gl2 only card, it would probably be removed someday, zerogs too).

In the end that not really important. It is just a matter of preference. In the past I read an interview of a Microsoft executive, it said that developers are free to code the way they like. No restrictive coding rule.

IMHO, the goal is readable code not beautiful code that depends on people mood or current hype.

[sudonim1]

@xsacha you were always able to comment, just not on the issue that I locked. Nobody without push permission can comment on locked issues.

We can add some code guidelines but you will be sorely disappointed by how lenient they will have to be.

[sudonim1]

Wait we already have a style guide. What the hell was this whole issue about?

[ramapcsx2]

As I see it, these people value those conventions much more than we do and thus consider it important enough for this issue :) We talked long and broad about this on IRC.