{"action":"created","issue":{"url":"https://api.github.com/repos/ppy/osu-performance/issues/1","repository_url":"https://api.github.com/repos/ppy/osu-performance","labels_url":"https://api.github.com/repos/ppy/osu-performance/issues/1/labels{/name}","comments_url":"https://api.github.com/repos/ppy/osu-performance/issues/1/comments","events_url":"https://api.github.com/repos/ppy/osu-performance/issues/1/events","html_url":"https://github.com/ppy/osu-performance/issues/1","id":149875678,"number":1,"title":"general remarks","user":{"login":"vendethiel","id":199499,"avatar_url":"https://avatars.githubusercontent.com/u/199499?v=3","gravatar_id":"","url":"https://api.github.com/users/vendethiel","html_url":"https://github.com/vendethiel","followers_url":"https://api.github.com/users/vendethiel/followers","following_url":"https://api.github.com/users/vendethiel/following{/other_user}","gists_url":"https://api.github.com/users/vendethiel/gists{/gist_id}","starred_url":"https://api.github.com/users/vendethiel/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vendethiel/subscriptions","organizations_url":"https://api.github.com/users/vendethiel/orgs","repos_url":"https://api.github.com/users/vendethiel/repos","events_url":"https://api.github.com/users/vendethiel/events{/privacy}","received_events_url":"https://api.github.com/users/vendethiel/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":32,"created_at":"2016-04-20T20:24:18Z","updated_at":"2016-06-27T09:59:33Z","closed_at":null,"body":"I don't feel confident enough in writing a PR since there are no tests for me to run & check if I broke stuff.\r\n\r\n[Src/Shared/Config](https://github.com/ppy/osu-performance/blob/d8db495725f6c0e3a13000ecac65fbe73ec1e730/Src/Shared/Config.cpp)\r\n\r\n- the spacing in the file seems **very* inconsistent (why so many blank lines?)\r\n- define/undefine macros directly in that file, not in `configVariables.h` (don't `ifndef` there).\r\n- inline-included files usually don't use \".h\" in C++. [`.inl` is a widely-used extension](http://stackoverflow.com/questions/1208028/significance-of-a-inl-file-in-c)\r\n- [your destructor can be defaulted](https://github.com/ppy/osu-performance/blob/d8db495725f6c0e3a13000ecac65fbe73ec1e730/Src/Shared/Config.cpp#L24). (`class A { ~A() = default; };`)\r\n- 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`)\r\n- [You don't need a `return;` after a `throw;`](https://github.com/ppy/osu-performance/blob/d8db495725f6c0e3a13000ecac65fbe73ec1e730/Src/Shared/Config.cpp#L48)\r\n- [There's a function to check if a char is a space](https://github.com/ppy/osu-performance/blob/d8db495725f6c0e3a13000ecac65fbe73ec1e730/Src/Shared/Config.cpp#L75), it's [`std::isspace`](http://en.cppreference.com/w/cpp/string/byte/isspace)\r\n- [Checking if a character is in a range](https://github.com/ppy/osu-performance/blob/d8db495725f6c0e3a13000ecac65fbe73ec1e730/Src/Shared/Config.cpp#L110-L113) is usally easier done with [`std::strchr`](http://en.cppreference.com/w/cpp/string/byte/strchr), i.e.: `std::strchr(\"aeiouy\", c) != nullptr`\r\n- Don't use `NULL`. use `nullptr`.\r\n- Don't use `strcmp`. Use `std::string`.\r\n- Use RAII instead of raw arrays (so that you can lose the `delete[]`).\r\n- `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)\r\n\r\n[Src/Shared/Core/Active](https://github.com/ppy/osu-performance/blob/d8db495725f6c0e3a13000ecac65fbe73ec1e730/Src/Shared/Core/Active.cpp)\r\n- if you can \"afford\" `std::make_unique`, use it (no big deal anyways).\r\n- [`foo == false`](https://github.com/ppy/osu-performance/blob/d8db495725f6c0e3a13000ecac65fbe73ec1e730/Src/Shared/Core/Active.cpp#L53) is better expressed as `!foo`.\r\n- 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...\r\n- don't use `NULL`; use `nullptr`\r\n- [any code after `std::rethrow_exception` is dead code /!\\ ](https://github.com/ppy/osu-performance/blob/d8db495725f6c0e3a13000ecac65fbe73ec1e730/Src/Shared/Core/Active.cpp#L94)\r\n\r\n[Src/Shared/Core/Exception](https://github.com/ppy/osu-performance/blob/d8db495725f6c0e3a13000ecac65fbe73ec1e730/Src/Shared/Core/Exception.cpp)\r\n- you can default your destructors.\r\n\r\n[Src/Shared/Core/Logger](https://github.com/ppy/osu-performance/blob/d8db495725f6c0e3a13000ecac65fbe73ec1e730/Src/Shared/Core/Logger.cpp)\r\n- 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).\r\n- `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.\r\n- [`auto a = std::string{};`](https://github.com/ppy/osu-performance/blob/d8db495725f6c0e3a13000ecac65fbe73ec1e730/Src/Shared/Core/Logger.cpp#L63) really is just `std::string a;`\r\n- [why do you convert your string to a c_str to call Write?](https://github.com/ppy/osu-performance/blob/d8db495725f6c0e3a13000ecac65fbe73ec1e730/Src/Shared/Core/Logger.cpp#L150)\r\n- `sizeof(char)` is 1.\r\n\r\n[Src/Shared/Core/StringUtil](https://github.com/ppy/osu-performance/blob/d8db495725f6c0e3a13000ecac65fbe73ec1e730/Src/Shared/Core/StringUtil.cpp)\r\n- 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\r\n\r\n[Src/Shared/Core/Threading](https://github.com/ppy/osu-performance/blob/d8db495725f6c0e3a13000ecac65fbe73ec1e730/Src/Shared/Core/Threading.cpp)\r\n- 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)).\r\n- [When you know the size you need, you can `.reserve` enough space](https://github.com/ppy/osu-performance/blob/d8db495725f6c0e3a13000ecac65fbe73ec1e730/Src/Shared/Core/Threading.cpp#L154).\r\n- [it doesn't make sense to move the return of a function](https://github.com/ppy/osu-performance/blob/d8db495725f6c0e3a13000ecac65fbe73ec1e730/Src/Shared/Core/Threading.cpp#L181). It's already a rvalue -- the compiler will do the moving for you. (yeah, the rules are really complex...).\r\n- you don't need a new scope for that last `unique_ptr` in \"StartThreads\" since RAII works LIFO (last in, first out).\r\n\r\n[Src/Shared/Network/DatabaseConnection](https://github.com/ppy/osu-performance/blob/d8db495725f6c0e3a13000ecac65fbe73ec1e730/Src/Shared/Network/DatabaseConnection.cpp).\r\n- you could move all your string parameters in the constructor\r\n- 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).\r\n- use `nullptr` instead of `NULL`.\r\n- `while (a != 0)` is `while (a)`\r\n\r\n[Src/Shared/Network/QueryResult](https://github.com/ppy/osu-performance/blob/d8db495725f6c0e3a13000ecac65fbe73ec1e730/Src/Shared/Network/QueryResult.cpp)\r\n- use `nullptr` instead of `NULL`.\r\n\r\nThat's pretty much all I saw for Src/Shared. I'll try to look at other files tomorrow :)"},"comment":{"url":"https://api.github.com/repos/ppy/osu-performance/issues/comments/228703491","html_url":"https://github.com/ppy/osu-performance/issues/1#issuecomment-228703491","issue_url":"https://api.github.com/repos/ppy/osu-performance/issues/1","id":228703491,"user":{"login":"vendethiel","id":199499,"avatar_url":"https://avatars.githubusercontent.com/u/199499?v=3","gravatar_id":"","url":"https://api.github.com/users/vendethiel","html_url":"https://github.com/vendethiel","followers_url":"https://api.github.com/users/vendethiel/followers","following_url":"https://api.github.com/users/vendethiel/following{/other_user}","gists_url":"https://api.github.com/users/vendethiel/gists{/gist_id}","starred_url":"https://api.github.com/users/vendethiel/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vendethiel/subscriptions","organizations_url":"https://api.github.com/users/vendethiel/orgs","repos_url":"https://api.github.com/users/vendethiel/repos","events_url":"https://api.github.com/users/vendethiel/events{/privacy}","received_events_url":"https://api.github.com/users/vendethiel/received_events","type":"User","site_admin":false},"created_at":"2016-06-27T09:59:33Z","updated_at":"2016-06-27T09:59:33Z","body":"oh, gotta change fast I guess."}}
{"actor":{"display_login":"vendethiel"}}