Refactor MyMoneyException class
AbandonedPublic

Authored by wojnilowicz on May 10 2018, 3:30 PM.

Details

Summary

Purpose of this patch is to simplify exception handling in KMyMoney by replacing MyMoneyException class with std::runtime_error. The advantage of this is that:

  1. we use code from standard library,
  2. we have less to compile,
  3. we catch all std::exceptions instead of MyMoneyException only.

mymoneyexception.h remains and contains header for std::runtime_error and two defines for customizing exception messages.
First define, MYMONEYEXCEPTION constructs the message roughly in the following way:

  1. construct QString from LINE, which is int,
  2. construct QString from FILE, which is const char *,
  3. take exception message, only QStrings allowed,
  4. construct QString from the three strings above,
  5. construct const char * from QString, for which std::runtime_error has a constructor.

As one can see, there is lot of effort to throw an exception, so there MYMONEYEXCEPTION_CSTRING has been introduced, to avoid all above mentioned memory allocations.

Diff Detail

Repository
R261 KMyMoney
Lint
Lint Skipped
Unit
Unit Tests Skipped
wojnilowicz requested review of this revision.May 10 2018, 3:30 PM
wojnilowicz created this revision.
christiand requested changes to this revision.EditedMay 12 2018, 12:14 PM
christiand added a subscriber: christiand.

Hi Łukasz,

actually I liked the separate class for exception handling. It allows to distinguish between issues within KMyMoney and outside of it. Typically we do not/cannot handle things like std::bad_alloc so the “missing” catch is desired. Also there is some code which expects this behavior (I think the onlineJob casting stuff), just replacing everything is dangerous. You could inherit MyMoneyException from std::runtime_error. Then we can knowingly change the catches where it is useful and safe.

I do not think that the reduced amount to compile is a benefit here. Reading these two sentences will for sure require more time than all users of KMyMoney together can possibly save.

kmymoney/mymoney/mymoneyexception.h
33

Change this to KMM_{TOSTRING,STRINGIFY} or alike because the used names are quite common which can cause issues.

61–63

You can make this obsolete by using an overloaded function. The marco calls the function and the overloading handles the QString or char* correctly.

kmymoney/mymoney/payeeidentifier/payeeidentifier.h
30

Here is the same about naming as above.

32

I like these marcos but they could be overpowered. There should be no place except this class (and the …typed version) where these exceptions are thrown.

kmymoney/plugins/onlinetasks/sepa/tasks/sepaonlinetransferimpl.cpp
160

To prevent this kind of code the payeeIdentifier::exception should be kept. This can even save time (now up to two cast are done, previously only one).

This revision now requires changes to proceed.May 12 2018, 12:14 PM
wojnilowicz marked 2 inline comments as done.May 12 2018, 1:13 PM

Hi Łukasz,

actually I liked the separate class for exception handling. It allows to distinguish between issues within KMyMoney and outside of it. Typically we do not/cannot handle things like std::bad_alloc so the “missing” catch is desired. Also there is some code which expects this behavior (I think the onlineJob casting stuff), just replacing everything is dangerous. You could inherit MyMoneyException from std::runtime_error. Then we can knowingly change the catches where it is useful and safe.

I do not think that the reduced amount to compile is a benefit here. Reading these two sentences will for sure require more time than all users of KMyMoney together can possibly save.

Hi Christian,

I understand that it allows us to distinguish between exceptions but who should handle exceptions that we can't handle?
I think it's our duty to do that, because some library throws it at us, because we acted on it in some way, so we are here to blame, so we should handle it.
Just passing exception beyond our application doesn't make the issue go away.
In my opinion, we should know every exception that is being thrown at us, so that we can be aware, even if we aren't prepared from the outset for it.

As for the second part of you concerns, logic of exceptions in onlineJob is left untouched. There are still two exceptions, which can be thrown, so no problem here.

I do not think that the reduced amount to compile is a benefit here. Reading these two sentences will for sure require more time than all users of KMyMoney together can possibly save.

I don't know what do you mean. Could you explain?

kmymoney/mymoney/mymoneyexception.h
33

Agreed.

61–63

AFAIK, I cannot overload #define. Adding own class makes it more complicated because you've got two constructors and additional code to compile, which doesn't give use more usability, which adds QString include to every compilation unit and which inlines the code in every compilation unit.

kmymoney/mymoney/payeeidentifier/payeeidentifier.h
32

So you want to remove these macros?

kmymoney/plugins/onlinetasks/sepa/tasks/sepaonlinetransferimpl.cpp
160

Do we need to catch two exceptions at all? Can't we just catch std::exception?

I understand that it allows us to distinguish between exceptions but who should handle exceptions that we can't handle?
I think it's our duty to do that, because some library throws it at us, because we acted on it in some way, so we are here to blame, so we should handle it.
Just passing exception beyond our application doesn't make the issue go away.
In my opinion, we should know every exception that is being thrown at us, so that we can be aware, even if we aren't prepared from the outset for it.

I understand you point of view and in most cases I agree with you. However, from many years of code development I learned that handling all exceptions right is just not feasible. Also it adds a lot of code for cases which nearly never happen. Just think about std::bad_alloc, I am sure we never handle that even though every new can throw it. Even if you want to handle it, how do you want to do that? In these cases it is way easier and justifiable just to quit and restart the program. Additionally I want to highlight that this patch does not handle all exceptions, it just pretends to do so — by catching them. Imaging for any reason no more memory can be allocated, then a std::bad_alloc is thrown. I am sure that the first one is catched but just to provoke a second one which is not handled anymore. Also, for good reasons C++11 deprecated the throws(…) construct (which required to know all exception possibly thrown) — it just never worked in real life.

I do not think that the reduced amount to compile is a benefit here. Reading these two sentences will for sure require more time than all users of KMyMoney together can possibly save.

I don't know what do you mean. Could you explain?

I just wanted to say (in a word-full way) that we do not need to bother about “we have less to compile”.

kmymoney/mymoney/mymoneyexception.h
61–63

What about something like:

#define MYMONEYEXCEPTION(what) makeMyMoneyExecption(what, __FILE__ ":" KMM_TOSTRING(__LINE__))

inline std::runtime_error makeMyMoneyException(QString what, char* location) {
    return std::runtime_error(qPrintable(what + QLatin1String(location));
}

inline std::runtime_error makeMyMoneyException(char* what, char* location) {
    return /* concatenate via QString or std::string */;
}

I would not look at time advantages too much, because they are for sure extremely low and this code is not called often.

kmymoney/mymoney/payeeidentifier/payeeidentifier.h
32

No. Now your work is done, so leave it.

kmymoney/plugins/onlinetasks/sepa/tasks/sepaonlinetransferimpl.cpp
160

Maybe, but we have to decide this for each catch individually. This is a lot of (error prone) work without a significant benefit.

wojnilowicz marked an inline comment as done.May 14 2018, 1:10 PM

I understand that it allows us to distinguish between exceptions but who should handle exceptions that we can't handle?
I think it's our duty to do that, because some library throws it at us, because we acted on it in some way, so we are here to blame, so we should handle it.
Just passing exception beyond our application doesn't make the issue go away.
In my opinion, we should know every exception that is being thrown at us, so that we can be aware, even if we aren't prepared from the outset for it.

I understand you point of view and in most cases I agree with you. However, from many years of code development I learned that handling all exceptions right is just not feasible. Also it adds a lot of code for cases which nearly never happen. Just think about std::bad_alloc, I am sure we never handle that even though every new can throw it. Even if you want to handle it, how do you want to do that? In these cases it is way easier and justifiable just to quit and restart the program. Additionally I want to highlight that this patch does not handle all exceptions, it just pretends to do so — by catching them. Imaging for any reason no more memory can be allocated, then a std::bad_alloc is thrown. I am sure that the first one is catched but just to provoke a second one which is not handled anymore. Also, for good reasons C++11 deprecated the throws(…) construct (which required to know all exception possibly thrown) — it just never worked in real life.

The way you presented the issue with std::bad_alloc convinced me. It seems like catching external exception isn't an advantage here. I just restored MyMoneyException, but didn't inherit it from std::runtime_error because it has lost all its advantages and juggling between "const char*" and "QString" isn't very sane as well, so what's left is:

  1. Untranslated excepctions, as normal user doesn't need to understand exceptions, only developers do,
  2. Tidied up catching phrase, so it's always catched by const reference,
  3. no d-pointer (there should be no cascade header inlusion) and reduced implementation to only what() method,
  4. QStringLiterals for exception messages.

I do not think that the reduced amount to compile is a benefit here. Reading these two sentences will for sure require more time than all users of KMyMoney together can possibly save.

I don't know what do you mean. Could you explain?

I just wanted to say (in a word-full way) that we do not need to bother about “we have less to compile”.

I care for that. Less code = more development time (most important) = less potential bugs = less maintenance. Why you think we shouldn't bother about that?

kmymoney/mymoney/mymoneyexception.h
61–63

That's another little doing functions. I think it's not good for simplification.
I agree, that time advantages at runtime are of low priority but time advantages at build time are of high priority to me. These functions would be inlined very often = increased build time (and probably size).

wojnilowicz retitled this revision from Get rid of MyMoneyException class to Refactor MyMoneyException class.
christiand resigned from this revision.May 16 2018, 7:30 PM

[…] so what's left is:

  1. Untranslated excepctions, as normal user doesn't need to understand exceptions, only developers do,

Here I agree with you. Unfortunately, they are used to inform the user anyway (violating this concept) and thus should be kept translated. In the long run this could be replaced by separate classes.

  1. Tidied up catching phrase, so it's always catched by const reference,

+1 (but be aware that the compiled code will not change, the optimizer should have noticed that already).

  1. no d-pointer (there should be no cascade header inlusion)

Very good! That was overpowered anyway.

and reduced implementation to only what() method,

I see this critical, because I do not see a benefit here. Now the string composition has to be done when thrown. However, in most cases this string is never used. I do not assume a performance advantage from this patch.

  1. QStringLiterals for exception messages.

I recommend not to do this either. This makes the use of the macro more complicated without a benefit. Here a macro which calls a overloaded constructor would be way easier to use.

I just wanted to say (in a word-full way) that we do not need to bother about “we have less to compile”.

I care for that. Less code = more development time (most important) = less potential bugs = less maintenance. Why you think we shouldn't bother about that?

I love less code!

At this point I recommend to abandon this patch. Maybe just start a new patch which removes the d-ptr (and just commit the added const in the catches without other changes).

kmymoney/mymoney/mymoneyexception.h
32–33

You removed the useful documentation. It should be kept and adopted.

kmymoney/mymoney/payeeidentifier/payeeidentifier.h
113

Is inheriting from MyMoneyException possible? As described in the deleted comment I could not manage to do so.

[…] so what's left is:

  1. Untranslated excepctions, as normal user doesn't need to understand exceptions, only developers do,

Here I agree with you. Unfortunately, they are used to inform the user anyway (violating this concept) and thus should be kept translated. In the long run this could be replaced by separate classes.

If they are violating the concept, then it should be made right. I think that any messages that inform the user should be in form of KMessageBox (as the name suggests) and not in form of MyMoneyException. Nevertheless I would like to fix all the situations in which exceptions are thrown as normal application routine like e.g. fetching security/currency from MyMoneyFile.

AFAIK an application could be compiled without exceptions and it should work as well.

  1. Tidied up catching phrase, so it's always catched by const reference,

+1 (but be aware that the compiled code will not change, the optimizer should have noticed that already).

I think, that we shouldn't depend on the compiler to fix our bad quality code.

  1. no d-pointer (there should be no cascade header inlusion)

Very good! That was overpowered anyway.

and reduced implementation to only what() method,

I see this critical, because I do not see a benefit here. Now the string composition has to be done when thrown. However, in most cases this string is never used. I do not assume a performance advantage from this patch.

  1. QStringLiterals for exception messages.

I recommend not to do this either. This makes the use of the macro more complicated without a benefit. Here a macro which calls a overloaded constructor would be way easier to use.

I feel, that you're giving me conflicting statements from one review to the other. Let's be practical and take advantages from only one approach, as we can't have them all.
You've made a good pointer, that exception message is built and most of the time is not used. In current master there are many places where allocated exception string (the ones with placeholder) is also built and then not used, so I consider that less important but nevertheless it brings me back to my previous solution.

I fell like making an error with this patch. It will cause to inline costly constructors (because of QString which is non-POD) in every place it's called. The same is true for what() method.
I should not abandon std::runtime_error in the first place, as it offers little dependency and light constructors.

I brought back MYMONEYEXCEPTION_CSTRING, so that I don't have to use QStringLiterals and build string every time an exception is called.

At this point I recommend to abandon this patch. Maybe just start a new patch which removes the d-ptr (and just commit the added const in the catches without other changes).

I don't want to abandon this patch. I know all your concerns and tried to take appropriate stand. I want to bring the code forward. I see that you've resigned from reviewers list, so I'm attaching it here for information and will commit it soon.

kmymoney/mymoney/mymoneyexception.h
32–33

I brought them back.

kmymoney/mymoney/payeeidentifier/payeeidentifier.h
113

I do that right now.

wojnilowicz abandoned this revision.May 19 2018, 5:54 AM