Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite the very real enterprise project of totally not a satire project to C++ (real) #694

Open
wants to merge 28 commits into
base: uinverse
Choose a base branch
from

Conversation

GEOEGII555
Copy link

@GEOEGII555 GEOEGII555 commented Jul 8, 2024

Hello,

At enterprise quality programming Co. LTD, performance is one of the top priorities that a company would have. The slower the performance is, the less clients a company would have. It's one of our top goals to maximize the performance.

That being said, while Java is a great programming language, it can't beat the speed of C and C++.

So, in order to maximize the performance of the fizz buzz: enterprise edition, and to beat the competitors, we need to rewrite Fizz Buzz: Enterprise Edition in C++.

While it's possible to write assembly code that has better performance than C++, it would cost too much. Most C++ compilers, such as the Microsoft MSVC compiler, generate very fast executables, where optimizing it further is not worth it.

Since we are on C++, our very serious enterprise project only supports the very serious enterprise platform called "Windows NT", we can leverage:

The native Windows API. Java has a translator which translates the instructions for a specific operating system. However, since we're only on Windows NT, we can avoid doing such conversions and instead directly use the Windows API.

We also don't have to worry about destroying the performance and significantly increasing the dependency count when leveraging:

The C++ standard library. It has a lot of useful things which don't destroy the performance, unlike Java. It also doesn't require a lot of dependencies (just an 11 gigibyte installation of Visual Studio) to run quickly and without performance issues.
I haven't listed all of the upsides of rewriting our very serious enterprise project to C++.

The only downside of this is that we will be using the Windows API, which means that if some company ever decides to use Linukz as their operating system, even though it's not a serious business environment, they won't be able to use our project, which will lead to revenue loss.

This pull request closes #693.

There's one question: do we add tests?

Copy link

@Capital-Asterisk Capital-Asterisk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things. would be cool to have CMake support and C++23 too.

argparser.cpp Outdated Show resolved Hide resolved
argparser.cpp Outdated Show resolved Hide resolved
argparser.hpp Outdated Show resolved Hide resolved
console.cpp Outdated Show resolved Hide resolved
fizz_buzz.hpp Show resolved Hide resolved
output_writer.cpp Outdated Show resolved Hide resolved
fizz_buzz.hpp Outdated Show resolved Hide resolved
console.cpp Show resolved Hide resolved
@GEOEGII555
Copy link
Author

GEOEGII555 commented Jul 15, 2024

A few things. would be cool to have CMake support and C++23 too.

We upgraded the project standard to C++23.

Thanks for the suggestion about using CMake, however, our very serious totally not satire company only uses serious business tools, such as Microsoft Visual Studio 2022, and not Linukz KitWare CMake.

@Capital-Asterisk
Copy link

There better be tests for std::basic_string; must be sure that they work properly

@GEOEGII555
Copy link
Author

Requesting a review because changes were done to src/code/README.md.

@Danmyrer
Copy link

@GEOEGII555 have you considered that C++ is older than Java? Shouldn't migration efforts be directed towards newer technologies? Company wants us to use the latest and most hyped technologies to keep our stack more competitive in the future.

@GEOEGII555
Copy link
Author

GEOEGII555 commented Nov 1, 2024

@GEOEGII555 have you considered that C++ is older than Java? Shouldn't migration efforts be directed towards newer technologies?

Please keep in mind that C++ is used to this day. I saw a popular commonly used project which is still on C++11 (August 2011)! Can't remember the exact name though. This PR uses a newer version of C++.

Company wants us to use the latest and most hyped technologies to keep our stack more competitive in the future.

If you look at the latest update release date, both are new. If you look at the date of the initial release, we should be moving away from Java to a new programming language no one else uses.

Currently, C++23 is used. It was released in December 2023, which is a year ago. It's relatively new compared to the C++ versions other commercial products use.

@PurHur
Copy link

PurHur commented Nov 1, 2024

We should consult some tech leads that will decide if raw c++ is secure enough for this software. I guess some parts could be written in rust to make is more secure.

@GEOEGII555
Copy link
Author

We should consult some tech leads that will decide if raw c++ is secure enough for this software. I guess some parts could be written in rust to make is more secure.

While I'm not a Rust developer in any kind, in my opinion, combining Rust and C++ would be more unsafe than using raw C++ or raw Rust. Also please keep in mind that Rust compilation is slow.

@jedwards1211
Copy link

Hmmm, needs some dependency injection via templates

Copy link

@rillig rillig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch looks good indeed. I just have a few stylistic remarks.

#include <vector>
#include "argparser.hpp"

TCHAR helpMessage[] = TEXT("Usage: [program] [--output file] [--enable_cache cache_file] [--source source]\n\
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the help message not marked constexpr? Having a text this large in a writeable memory page may impact performance, in case the page needs an extra copy-on-write.

\t\ttestinput - A fake input source. It inputs all numbers from 1 to 100 (both sides included).\n");

// See the help message above for more info about these flags.
constexpr TCHAR HELP_FLAG[] = TEXT("--help");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to add static here to avoid the external linkage? That should result in a smaller executable file.

else if (lstrcmp(*it, OUTPUT_FILENAME_FLAG) == 0) {
std::advance(it, 1);
if (it >= argvVector.end()) {
showHelp = true;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be an error indicator, as a plain --help should exit successfully, while this case should exit erroneously.

#include <algorithm>
#include <ranges>

_console::_console() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For internationalization, it may be useful to switch the console window to UTF-8. I did a proof of concept a few years ago. My approach is not very enterprisey, but maybe you can convert it to something more professional.

abort();
}
unsigned long long int fileSize;
GetFileSizeEx(this->hFile, (PLARGE_INTEGER) & fileSize);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handling is missing here.

if (temp == 0) {
_this->inputExhausted = true;
throw std::exception("There's nothing more in the input.");
return -1;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return statement is redundant.


// Welcome to 2024, where open source projects have a copyright.
TCHAR copyrightMessage[] = {
TEXT("FizzBuzz Enterprise Edition.\nEnterpriseQualityCoding - the most serious and not satire or fake company in the uinverse.\n")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: uinverse -> universe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Achieve better performance by rewriting to C++
6 participants