general remarks

[vendethiel]

I don't feel confident enough in writing a PR since there are no tests for me to run & check if I broke stuff.

Src/Shared/Config

• the spacing in the file seems *very inconsistent (why so many blank lines?)
• define/undefine macros directly in that file, not in configVariables.h (don't ifndef there).
• inline-included files usually don't use ".h" in C++. .inl is a widely-used extension
• your destructor can be defaulted. (class A { ~A() = default; };)
• C++ has other APIs for opening files, other than fopen/fseek/ftell/etc, but maybe that's for performance reasons? (Even then, you should try to use some other construct to get RAII fclose)
• You don't need a return; after a throw;
• There's a function to check if a char is a space, it's std::isspace
• Checking if a character is in a range is usally easier done with std::strchr, i.e.: std::strchr("aeiouy", c) != nullptr
• Don't use NULL. use nullptr.
• Don't use strcmp. Use std::string.
• Use RAII instead of raw arrays (so that you can lose the delete[]).
• sizeof(char) is 1, as per standard (sizeof returns the number of chars you can stuff in one type... so it's always gonna be 1 for char)

Src/Shared/Core/Active

• if you can "afford" std::make_unique, use it (no big deal anyways).
• foo == false is better expressed as !foo.
• any reason to catch (...) and assigning the exception, inside of using your destructor with std::uncaught_exception(s) (the behavior of either is very different, and the second one is C++17 only)? The problem is that this is dangerous... on windows, you can catch a lot more than you might first except if SEH exceptions are enabled - that includes access violations...
• don't use NULL; use nullptr
• any code after std::rethrow_exception is dead code /!\

Src/Shared/Core/Exception

• you can default your destructors.

Src/Shared/Core/Logger

• don't take a std::string&& to forward it. Either do that, and std::move, or just do it the "simple" way: take it by value (and then move it).
• CLog::Log takes the text by rvalue reference, but then copies the value inside the lambda. If you enabled C++14, take it by value and use [Text=std::move(Text)]() { ... } to move it inside the lambda.
• auto a = std::string{}; really is just std::string a;
• why do you convert your string to a c_str to call Write?
• sizeof(char) is 1.

Src/Shared/Core/StringUtil

• If you take your param by non-const ref, don't return it as well. (especially since your 2nd overload doesn't use the return value). That's true for split's first overload, and for the trim functions

Src/Shared/Core/Threading

• That's some pretty... hairy code? I mean, mutexes named L N M makes it pretty hard (for me) to reason through it. I'm not sure you couldn't use some C++11/C++14/Boost mutex type? (C++17 and Boost both have a shared mutex impl (i.e. multi reader, only one writer)).
• When you know the size you need, you can .reserve enough space.
• it doesn't make sense to move the return of a function. It's already a rvalue -- the compiler will do the moving for you. (yeah, the rules are really complex...).
• you don't need a new scope for that last unique_ptr in "StartThreads" since RAII works LIFO (last in, first out).

Src/Shared/Network/DatabaseConnection.

• you could move all your string parameters in the constructor
• instead of using new MYSQL, use a unique_ptr with a custom deleter (that's gonna be mysql_close). You can remove all the delete this way (and use .reset() where needed).
• use nullptr instead of NULL.
• while (a != 0) is while (a)

Src/Shared/Network/QueryResult

• use nullptr instead of NULL.

That's pretty much all I saw for Src/Shared. I'll try to look at other files tomorrow :)

[lauriys]

that was fast

[vendethiel]

i'm a well-known hater, so I gotta hate, right?

[peppy]

Just gonna mention that the main reason this was open sourced was to get feedback/improvements on the pp algorithm itself.

[vendethiel]

I know next to nothing about maths, sorry :).

[peppy]

That's fine, and your contribution is definitely valuable. Just wanted to state our focus here in case it takes a bit of time to have these kinds of things addressed.

[Tom94]

Hi there, yup, most of the code has been written years ago and I did not bother getting it up to speed, since it "worked". There are some good finds and suggestions in your remarks as well, thanks a lot for your effort!

Even if you are worried, feel free to make a PR with these changes. We/I won't pull it in without verifying it works. :)
Would be much appreciated, since I have extremely limited time to work on this project.

[Tom94]

To go into your feedback in more detail. I agree with all points I do not explicitly mention.

General:

• I prefer using sizeof(char) instead of 1 in situations where the code would otherwise break if you replaced char by some other, longer type. If you use 1, then you may more easily break your code while refactoring.

Src/Shared/Config: Super old code, needs a rewrite anyway. I agree with your comments
Src/Shared/Core/Active:

• std::make_unique is C++14. I'm not sure if I want this program to be incompatible with C++11 just yet.
• NULL was only used to interface with the windows.h API, which uses NULL all over the place. The rationale was to be consistent with this API. Although, using nullptr may arguably be more elegant, not 100% sure on that one.
• Are you suggesting the usage of std::uncaught_exception? I don't think it's already used.

Src/Shared/Core/Logger: Again, old code, needs to be refurbished. :)

• auto s = std::string{}; rationale came from Herb Sutter's Almost Always Auto . Not really sure if I like it or not to be honest.
• As said above, I'd like to stay in C++11 land for now. The unnecessary rvalue references need to go, though. These moves are mostly premature optimization (done wrong, even).

Src/Shared/Core/StringUtil: Returning the non-const ref is done so that these function calls can be chained, rather than be performed in sequence. Looks more elegant to me, although, I admit, it does involve some (negligible) unnecessary overhead. Are there any serious ways this could break, which are not present when returning void?

Src/Shared/Core/Threading:

• The hairy code was just some random experiment with code taken from StackOverflow. I don't think it's even used anyway. Gotta remove it. :)
• .reserve is most likely just premature optimization. I doubt the amortized O(1) allocation cost of std::vector will ever become the bottleneck over the non-amortized O(1) of .reserve in this particular case.
• Scopes having a deterministic destruction ordering is a good tidbit to know. I think I never heard that before, thanks.

Src/Shared/Network/DatabaseConnection: Again, horribly old code. Needs a rewrite or serious refactoring (on top of what it already received).

Src/Shared/Network/QueryResult: Same as above. They belong together.

[vendethiel]

I prefer using sizeof(char) instead of 1

My advice would rather be "replace N * sizeof(char) with N", but this also makes sense.

std::make_unique is C++14. I'm not sure if I want this program to be incompatible with C++11 just yet.

Okay, fair. Not sure why you're using brace initializers at time (i.e. there), though.

NULL was only used to interface with the windows.h API, which uses NULL all over the place. The rationale was to be consistent with this API. Although, using nullptr may arguably be more elegant, not 100% sure on that one.

You can safely use nullptr anyways. From the standard, 4.10: "A null pointer constant is an integer literal ([lex.icon]) with value zero".

Are you suggesting the usage of std::uncaught_exception? I don't think it's already used.

Really, it's up to you. Both have pros and cons (SEH exceptions are very much a WAT).

auto s = std::string{}; rationale came from Herb Sutter's Almost Always Auto . Not really sure if I like it or not to be honest.

I don't think auto a = T{}; is any more readable than T a;, tbh. Worse, you might confuse people into thinking there's some copy being done, etc.

As said above, I'd like to stay in C++11 land for now. The unnecessary rvalue references need to go, though. These moves are mostly premature optimization (done wrong, even).

Right. Sticking with the "take by value and move" is usually the 'simplest' thing to do. Only use std::forward when you're dealing with forwarding references (sometimes called "universal references"). That means a context where you have type deduction and reference collapsing (templates, auto, etc. Scott Meyers has a very good talk about that, btw). (also, "perfect forwarding" isn't perfect, because C++).

Are there any serious ways this could break, which are not present when returning void?

The non-confusing way to do this... Would be to take the strings by value :). Since a call is a rvalue, the compiler will do the move by himself (and often even optimize it out).

Here's an example. I don't think it makes that much sense to take a mutable reference – just take by value and return a temporary. Also, the value version would allow rtrim(ltrim(std::string("foo")) to work.

[Tom94]

Brace initializers were introduced with C++11, not 14, that's why I am using them.

The rest of what you've written makes sense. I'd be more than happy to accept pull requests addressing this stuff. Otherwise, I'll probably do it myself eventually when I have a larger amount of free time somewhere in the future. Thanks again.

[vendethiel]

.reserve is most likely just premature optimization. I doubt the amortized O(1) allocation cost of std::vector will ever become the bottleneck over the non-amortized O(1) of .reserve in this particular case.

There's no real reason to not use reserve, except for "I don't care".
.reserve is O(N), not O(1). By using it, you pay the allocation cost upfront, instead of each time the vector decides it has to realloc.
Remember also – if the vector reallocates, the compiler will have to move the objects, and that's another cost you have to pay.

NB: It's only possible for std::vector to use noexcept move constructors (strong exception guarantee), otherwise it has to fallback on copying.

[vendethiel]

I'd be more than happy to accept pull requests addressing this stuff.

When I say that "I don't feel confident enough", it mostly stems from the fact I have no Windows dev env. to work on, so... :P.

Brace initializers were introduced with C++11, not 14, that's why I am using them.

I gathered as much – I just mean "since you're not using the std::initializer_list constructor overload, it doesn't make that much sense to use them"

[Tom94]

When talking about runtime complexity I was talking about the cost for each element, which indeed is O(1) in the case of reserve. (You allocate N elements in O(N) time.)

The worst case you are describing (copying existing elements) still results in an amortized O(1) allocation cost per element due to a smart strategy about when and how to re-allocate. Note the "amortized". Even when taking into account copying of all existing elements! Fancy maths. ;)

So yeah, complexity-wise .reserve only gets rid of the "amortized". Of course it does speed things up by some constant factor. Otherwise there wouldn't be a need for it to exist. The reason I see against using it on every single vector where the size is known upfront is just, that it'll be one more line of code that can break some time in the future without yielding any tangible benefit. I'd really only use it when the vector is actually huge (millions of elements).

[vendethiel]

As I said, it's just fine "not to care", and I don't know how big that vector is supposed to be :). I just wanted to make it explicit that the cost of moves is not included in the complexity.

[Tom94]

The cost of moves / copys is included. See this.

[griwes]

I prefer using sizeof(char) instead of 1

sizeof(char) is literally defined to be 1 by the standard.

[Tom94]

griwes: Did you even read what I wrote after the text you quoted? You are not providing a counterargument at all.

[griwes]

@Tom94: I'm just pointing out, again, why you are spewing nonsense.

[griwes]

Also lol, there's no tests - that makes refactoring impossible.

[vendethiel]

@griwes the sizeof(char) thing was discussed about earlier, so these messages are not interesting :).

[Weikardzaena]

I'm going to post this comment here instead of opening a new issue because it is kinda general. I'm seeing a lot of arbitrary float values placed everywhere in the calculations of things.

E.G. from Src/Processor/User.cpp lines 55-56

// This weird factor is to keep legacy compatibility with the diminishing bonus of 0.25 by 0.9994 each score
_rating.Value += (417.0 - 1.0 / 3.0) * (1.0 - pow(0.9994, _scores.size()));

I know every one of the values in the source have been changed a lot in testing and are very precise, but having oodles of numbers without names makes it difficult to maintain, especially if you're unfamiliar with the code base. Using the example above, IMHO, it would be much easier to read if it simply was refactored to something like this:

const f32 WEIRD_DIMINISHING_SCORE_FACTOR = (417.0 - 1.0 / 3.0);
const f32 WEIGHT_PER_SCORE = 0.9994;

/* ... */

// This weird factor is to keep legacy compatibility with the diminishing bonus of 0.25 by 0.9994 each score
_rating.Value += WEIRD_DIMINISHING_SCORE_FACTOR * (1.0 - pow(WEIGHT_PER_SCORE, _scores.size()));

I'm of the opinion that there should never be a naked number in your code, and I'm wondering if there was a specific reason for all these numbers aside from "never got around to it"? If there wasn't a reason, the next question would be: would you be comfortable letting us propose a naming system for these numbers to more easily balance the numbers in the future?

[peppy]

i'm okay with adding named constants wherever possible. if you remove the WEIRD prefix you could probably post a PR and we'd be able to merge it in.

you have to understand that while we did open source this, it wasn't originally written to be the "perfect" code base. you are welcome to help make it that way, but it isn't something we had the man hours allocated towards :).

[Weikardzaena]

Awesome! Yeah that silly name was just to make a point. I would make something a bit more serious with the PR. Do you guys have an IRC or something set up so I can ask questions about specific numbers that I'm confused about when naming these constants?

[peppy]

You can join us on slack https://osu.ppy.sh/p/slack

[Tom94]

I don't think named constants for every formula used in the calculations make sense. The example you gave is completely fine (except that I'd move the comment above the actual values being defined, since it does directly reference them, and that I'd define the values right before they are actually used, rather than at the top of the function).

In the case of, for instance,
_aimValue *= pow(0.97f, _amountMiss);
i don't think
static const f32 PER_MISS_DECAY_FACTOR = 0.97f; _aimValue *= pow(PER_MISS_DECAY_FACTOR, _amountMiss);
is any more readable. If the constant is defined anywhere else than next to its usage, then it'll actually become more cumbersome to read and understand.

Furthermore, it kind of obfuscates the volatile nature of the entire function. When balancing you don't necessarily just want to tweak constants. Usually you want to change the function itself.

[Weikardzaena]

I guess that's a valid point. I'll just kinda keep making changes where it looks like it'll make sense and if it's too obfuscating then the PR can be revised. And perhaps the better solution to some of these formulas is just to add a descriptive comment about what each part of the formula does.

[VIctorianTchaikovsky]

Just pointing out that if you're using named constants be sure to use constexpr

Also I have a opinion ( though probably not shared ) that all caps should only be used on preprocessor-related stuff and therefore even constants shouldn't be all caps ( because I hate all caps )
It can be made clear that they are constants by shoving them into namespaces

namespace Constant{
    constexpr f32 Pi = 3.1415926535;
}
f32 areaOfCircle(f32 radius){
    return Constant::Pi * radius * radius;
}

[vendethiel]

I'm not sure you're not overcomplicating things just for the sake of it here...

[VIctorianTchaikovsky]

Ignore the latter half if you will but really, constexpr is awesome

[vendethiel]

It doesn't serve any purpose here. None of that is useful as a compile-time values. The whole "move values to constants" seems pretty moot to me

[vendethiel]

You can join us on slack https://osu.ppy.sh/p/slack

Is that not a thing anymore?

The public slack community is temporarily unavailable. If you wish to reach out, please create an issue on the appropriate github repository or contact us at accounts@ppy.sh.

[lauriys]

they moved to discord

[vendethiel]

oh, gotta change fast I guess.

[vendethiel]

std::make_unique is C++14. I'm not sure if I want this program to be incompatible with C++11 just yet.

Actually, you're already using make_unique

The hairy code was just some random experiment with code taken from StackOverflow. I don't think it's even used anyway. Gotta remove it. :)

I got to see that when I opened the file into vim (^M, ^M everywhere!).

So I ended up refactoring stuff pretty conservatively (I didn't touch the 2 parts that had windows line endings, say).