olympus-robotics / hephaestus

Hephaestus is a C++ framework designed to facilitate robotics development by providing commonly needed functionality and abstractions.
MIT License
18 stars 2 forks source link

[cmake] disable exception flag #108

Closed floriantschopp closed 2 weeks ago

floriantschopp commented 2 weeks ago

To be able to comply with google style, no exceptions should be thrown in the code (https://google.github.io/styleguide/cppguide.html#Exceptions). Therefore, I added an option to disable exceptions and replace with the CHECK macro of abseil.

johannes-graeter commented 2 weeks ago

Great! so the exceptions will effectively be turned into what other languages call a panic right? Perhaps it would then make sense to rename "throwException" to "panic" to empahsize this.

filippobrizzi commented 2 weeks ago

Great! so the exceptions will effectively be turned into what other languages call a panic right? Perhaps it would then make sense to rename "throwException" to "panic" to empahsize this.

But then when you are actually throwing exception it is not clear

johannes-graeter commented 2 weeks ago

Great! so the exceptions will effectively be turned into what other languages call a panic right? Perhaps it would then make sense to rename "throwException" to "panic" to empahsize this.

But then when you are actually throwing exception it is not clear

True, this would IMO only make sense, when not catching the exceptions in most cases (like go that has "recover" in cases you really want to recover from a panic...). On the other hand, it is strange when throwException does not emit an exception (caused by a compiler flag, which might not be visible from the code). So I guess a different name would be appropriate. In absence of a better idea I would suggest maybeThrowException :) Since I am missing some of the context, it might also too much nitpicking from my side ;)

filippobrizzi commented 2 weeks ago

Great! so the exceptions will effectively be turned into what other languages call a panic right? Perhaps it would then make sense to rename "throwException" to "panic" to empahsize this.

But then when you are actually throwing exception it is not clear

True, this would IMO only make sense, when not catching the exceptions in most cases (like go that has "recover" in cases you really want to recover from a panic...). On the other hand, it is strange when throwException does not emit an exception (caused by a compiler flag, which might not be visible from the code). So I guess a different name would be appropriate. In absence of a better idea I would suggest maybeThrowException :) Since I am missing some of the context, it might also too much nitpicking from my side ;)

@johannes-graeter Given that the default behaviour is to throw exceptions and to switch to the other behaviour you need to set that you want exceptions disabled explicitly I would keep this name. If you have manually disabled exception you know that you are not really throwing.

johannes-graeter commented 2 weeks ago

Great! so the exceptions will effectively be turned into what other languages call a panic right? Perhaps it would then make sense to rename "throwException" to "panic" to empahsize this.

But then when you are actually throwing exception it is not clear

True, this would IMO only make sense, when not catching the exceptions in most cases (like go that has "recover" in cases you really want to recover from a panic...). On the other hand, it is strange when throwException does not emit an exception (caused by a compiler flag, which might not be visible from the code). So I guess a different name would be appropriate. In absence of a better idea I would suggest maybeThrowException :) Since I am missing some of the context, it might also too much nitpicking from my side ;)

@johannes-graeter Given that the default behaviour is to throw exceptions and to switch to the other behaviour you need to set that you want exceptions disabled explicitly I would keep this name. If you have manually disabled exception you know that you are not really throwing.

@filippobrizzi I see your point and agree that this is good for now since users compile hephaestus themselfs. I still think this might open some problems in the future, since the contract for error handling depends on compiler flags instead of code. If released in a compiled state, it should be certain, that this gets shipped only with one version of the state. Users could otherwise have a hard time finding out why their recovery (from a wrong filename with try/catch f.e. ) results in a fatal...

johannes-graeter commented 2 weeks ago

Thanks for your patience @floriantschopp and @filippobrizzi, error handling is a very dear topic to me so I am very picky about it(since I have wasted soooo many hours coping with error handling strategies of external libs...)

filippobrizzi commented 2 weeks ago

Great! so the exceptions will effectively be turned into what other languages call a panic right? Perhaps it would then make sense to rename "throwException" to "panic" to empahsize this.

But then when you are actually throwing exception it is not clear

True, this would IMO only make sense, when not catching the exceptions in most cases (like go that has "recover" in cases you really want to recover from a panic...). On the other hand, it is strange when throwException does not emit an exception (caused by a compiler flag, which might not be visible from the code). So I guess a different name would be appropriate. In absence of a better idea I would suggest maybeThrowException :) Since I am missing some of the context, it might also too much nitpicking from my side ;)

@johannes-graeter Given that the default behaviour is to throw exceptions and to switch to the other behaviour you need to set that you want exceptions disabled explicitly I would keep this name. If you have manually disabled exception you know that you are not really throwing.

@filippobrizzi I see your point and agree that this is good for now since users compile hephaestus themselfs. I still think this might open some problems in the future, since the contract for error handling depends on compiler flags instead of code. If released in a compiled state, it should be certain, that this gets shipped only with one version of the state. Users could otherwise have a hard time finding out why their recovery (from a wrong filename with try/catch f.e. ) results in a fatal...