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.

Sign up for GitHub

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

Jump to bottom

Fixing memory leaks using exceptions #2620

Closed
wants to merge 11 commits into from
Closed

Fixing memory leaks using exceptions #2620

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2ecdb7f
Fixing memory leaks.
pgScorpio Apr 26, 2022
affd49d
Bugfix pClientRpc does not exist in server-only mode
pgScorpio Apr 26, 2022
e090359
Bugfix headless mode
pgScorpio Apr 26, 2022
e6b31f3
Solving some memory leaks again, but now using exceptions to exit.
pgScorpio Apr 26, 2022
ae873c1
Bugfixes pApp pointer type and exit code
pgScorpio Apr 27, 2022
5d0cf53
Buffix wrong pointertype pApplicationInstance
pgScorpio Apr 27, 2022
14128cb
Bugfix macOS
pgScorpio Apr 27, 2022
271d2b1
Optimizing exception handling
pgScorpio Apr 28, 2022
d1aad2a
using less #ifdef's by using defines for QApplication in headless mode
pgScorpio Apr 29, 2022
d04c899
Update src/main.cpp
pgScorpio Apr 30, 2022
de39150
Update src/main.cpp
pgScorpio Apr 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 40 additions & 2 deletions src/global.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,11 +319,41 @@ typedef unsigned char uint8_t;
// definition for custom event
#define MS_PACKET_RECEIVED 0

/* Classes ********************************************************************/
/* Exception Classes **********************************************************/

class CInfoExit
ann0see marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't add this class. It's only used to replace exit(0); from main, which should just be return 0;. It's bad style in C++ to use exceptions for handling normal situations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's bad style in C++ to use exceptions for handling normal situations.

It's what you call a "normal" situation...
But I agree that, just for the --help and --version, it could be a return ( 0 ).

{
public:
CInfoExit ( QString strMsg ) : strInfoMsg ( strMsg ) {}

QString GetInfoMessage() const { return strInfoMsg; }

protected:
QString strInfoMsg;
};

class CErrorExit
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd call this something like CCmdLineErr, to indicate what it's actually being used to report.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd call this something like CCmdLineErr, to indicate what it's actually being used to report.

Again, this is a 'universal' solution, so not only usable for commandline errors, so why name it that way just because we now use it for commandline errors?

Copy link
Member

Choose a reason for hiding this comment

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

I assume we can leave it as is - if we use it in future (and I think there is work coming up by @pgScorpio which uses it)

{
public:
CErrorExit ( QString strNewErrorMsg, int iNewExitCode = 1 ) : strErrorMsg ( strNewErrorMsg ), iExitCode ( iNewExitCode ) {}

QString GetErrorMessage() const { return strErrorMsg; }

int GetExitCode() const { return iExitCode; }
Copy link
Contributor

Choose a reason for hiding this comment

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

The exit code is always 1 on error in the code below, so iExitCode isn't needed. (You might want to add this in the future if there's really a good reason to have different exit codes, but I think that's very unlikely to be useful in Jamulus, and it's certainly not needed now.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The exit code is always 1 on error in the code below, so iExitCode isn't needed. (You might want to add this in the future if there's really a good reason to have different exit codes, but I think that's very unlikely to be useful in Jamulus, and it's certainly not needed now.)

I agree the exit code is (strangely enough) always 1 in this code, but as said we should provide a framework with general solutions, not prohibiting any 'normal' behavior.
Prohibiting any 'normal' behavior now will possibly lead to unwanted 'solutions' in the future.


protected:
QString strErrorMsg;
int iExitCode;
};

class CGenErr
{
public:
CGenErr ( QString strNewErrorMsg, QString strNewErrorType = "" ) : strErrorMsg ( strNewErrorMsg ), strErrorType ( strNewErrorType ) {}
CGenErr ( QString strNewErrorMsg, QString strNewErrorType = "", int iNewExitCode = 1 ) :
strErrorMsg ( strNewErrorMsg ),
strErrorType ( strNewErrorType ),
iExitCode ( iNewExitCode )
{}

QString GetErrorText() const
{
Expand All @@ -338,11 +368,16 @@ class CGenErr
}
}

int GetExitCode() const { return iExitCode; }

protected:
QString strErrorMsg;
QString strErrorType;
int iExitCode;
};

/* Event Classes **************************************************************/

class CCustomEvent : public QEvent
{
public:
Expand All @@ -359,6 +394,9 @@ class CCustomEvent : public QEvent
};

/* Prototypes for global functions ********************************************/

Copy link
Contributor

Choose a reason for hiding this comment

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

Not used - remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, Provide a Framework that should (and will) be used in the near future...

extern const QString GetAppName();

// command line parsing, TODO do not declare functions globally but in a class
QString UsageArguments ( char** argv );

Expand Down
Loading