thcrap_update: Use enum class, using rather than typedef, fix throw

[mikomikotaishi]

This PR aims to address the following:

• Some C++ (non-C) code uses unscoped enums (enum) rather than enum class, which is more consistently used in thcrap_update; we could switch to these for type safety.
• There is not really much reason to use typedefs over using aliases in C++, and using statements are much cleaner.
• At one point, a throw new invalid_argument() was used, which is not only an unnecessary heap allocation but even a source of memory leaks, and actually throws a pointer (which gets missed by most catch blocks)
• Catching exceptions by const reference is safer as it avoids unnecessary copies and avoids object slicing risks entirely.
• Where possible, static_cast<T>(x) is safer than using a C-style cast ((T)x). We won't use C++ casts.

[zero318]

Two notes:

• Using enum class looks fine here, though it would be nice to reformat things a bit to avoid the HttpStatus::Status redundancy that wasn't wasn't there before.
• I would like to avoid C++ style casting here. They add a lot of extra visual clutter and C style casting is only a major problem when dealing with polymorphic types, which doesn't apply to int

[mikomikotaishi]

Fine, we can remove the C++ casts.

Could you explain in more detail what you mean with HttpStatus::Status redundancy? I assume you mean that you want to avoid writing things like HttpStatus::Status::ServerError; I moved things to enum class because we'd be better off having strict typing on the enumerators, but using enum is a C++20 feature (while the project seems to be C++17). I guess we could do using HttpStatusOf = HttpStatus::Status; to bring it into scope and just write HttpStatusOf::ServerError, if that's fine too.

[zero318]

Could you explain in more detail what you mean with HttpStatus::Status redundancy?

Just the fact that HttpStatus::Status feels silly because it says Status twice. Maybe if the overall type were HttpResponse or something similar so that it's HttpResponse::Status? IDK if that's correct terminology though since web/http stuff isn't really my thing. Maybe @brliron has an opinion?

Few other minor things now that I'm reading over the changes again:

• Using NULL/TRUE/FALSE in loader_update.cpp was done intentionally because Win32 is a C API and not C++. I know it'll resolve correctly on any sane system, but when the docs say "If this parameter is NULL" it's nice to just do what it says. The thcrap_inject_into_new call could totally still be nullptr since that's our API and not Win32, but a consistent style within a single file is nice too.
• self.cpp/file.cpp/http_status.cpp seem to have minor regressions as part of resolving conflicts with my last few commits.
  • The unexpected macro on the if statement in file.cppgot removed. MSVC is a very dumb compiler and that helps it generate better code for handling rare error conditions.
  • Explicitly calling SendMessageW instead of SendMessage in self.cpp was done on purpose because we already don't care about the UNICODE macro and using macro versions of functions gives less helpful hover tooltips in visual studio. Macros only show #define SendMessage SendMessageW instead of the full list of parameters.
  • The s suffixes for std::string literals got removed in http_status.cpp. Without that MSVC is too dumb to precalculate the length of strings when constructing std::string from normal string literals because it converts them to const char* in the constructor.

[brliron]

Just the fact that HttpStatus::Status feels silly because it says Status twice. Maybe if the overall type were HttpResponse or something similar so that it's HttpResponse::Status? IDK if that's correct terminology though since web/http stuff isn't really my thing. Maybe @brliron has an opinion?

I remember thinking about using an enum class here when I initially wrote it, and explicitly deciding to use a regular enum because HttpStatus::Status::Something was too silly and verbose.

Here, I think the name "status" to represent the HTTP status code is correct. "response" would correspond to the whole response, with the whole header and also the content. This enum is a simplified representation by removing a level of detail we don't really care about.

Anyway, the cast at https://github.com/mikomikotaishi/thcrap/blob/9b69f2045b71654b7da17b35e8ddb75255f54cc9/thcrap_update/src/file.cpp#L112 made me notice that the HttpStatus is part of the C API. Instead of making it more C++ by turning it into an enum class, I think we need to make it less C++ by putting it in the global namespace with all caps values, put the C++ parts of the file into a #ifdef __cplusplus, and include it properly instead of redefining it in

thcrap/thcrap_wrapper/src/install_modules.c

Line 176 in 1a17df9

// From thcrap_update/src/http_status.h

.

[zero318]

Is there any particular reason for those files to be C instead of C++? This feels like something that should be considered an internal API rather than the external C API

[brliron]

There is one place where another component wants to use it:

thcrap/thcrap_wrapper/src/install_modules.c

Line 211 in 1a17df9

HttpStatus res = download_single_file("https://download.visualstudio.microsoft.com/download/pr/7afca223-55d2-470a-8edc-6a1739ae3252/abd170b4b0ec15ad0222a809b761a036/ndp48-x86-x64-allos-enu.exe", "NDP480-Installer.exe");

[mikomikotaishi]

Using NULL/TRUE/FALSE in loader_update.cpp was done intentionally because Win32 is a C API and not C++. I know it'll resolve correctly on any sane system, but when the docs say "If this parameter is NULL" it's nice to just do what it says. The thcrap_inject_into_new call could totally still be nullptr since that's our API and not Win32, but a consistent style within a single file is nice too.

I can revert those, I guess that makes sense.

self.cpp/file.cpp/http_status.cpp seem to have minor regressions as part of resolving conflicts with my last few commits.

OK, let me address these.

I think we need to make it less C++ by putting it in the global namespace with all caps values, put the C++ parts of the file into a #ifdef __cplusplus, and include it properly instead of redefining it in

Do you want me to go through with that on this PR? I feel like this would be more of a follow-up task.

[mikomikotaishi]

OK, I went ahead and restored the unexpected() macro (even though it certainly ought to be changed at some point), restored the Windows TRUE/FALSE macros, used SendMessageW(), and restored the ""s literals.

[brliron]

I have to ask... Are you an AI or a human delegating all your work to one? Human programmers tend to at least test whether their changes compile before committing them, and ideally test it as well - especially on a PR. Humans do make mistake, I did push code that our CI failed to build a few times, but looking at our CI history, 4 builds on this PR failed and only 2 succeeded.

[mikomikotaishi]

That doesn't make any sense to me; none of my changes introduce logical changes to the code. I typed every change I made here, and none of the changes I made really need an AI to complete. It's completely beyond me as to what the cause of the CI failures are. I noticed the mistake, but I do wish to clarify that it was made not as a result of using AI tools, but rather me mistakenly believing the changes were small/inconsequential enough to make without verifying through a build.

[brliron]

So you never downloaded the source code and built it with Visual Studio in order to make sure it at least builds properly? You didn't run the program and make sure the downloader's progress bar was still working properly after the SendMessageW -> SendMessage change?

[mikomikotaishi]

I made my edits from Linux and currently do not have access to a Windows device let alone Visual Studio, and I did not believe the changes would logically alter the program enough to cause regression, which is why I hadn't tested building. I apologise, but now that the changes do indeed build I do wish to stand by my expectations of no foreseeable regressions.

[mikomikotaishi]

@brliron Hi, is there still anything left on this PR that is objectionable? The CI builds fine, and aside from the throw new -> throw change and the catching by reference (these being actual correctness improvements), nothing logically differs in the program.