search

Derecho-Project / derecho

The main code repository for the Derecho project.
BSD 3-Clause "New" or "Revised" License
187 stars 47 forks source link

Macros make lf.cpp very hard to read and debug #151

Closed etremel closed 4 years ago

etremel commented 4 years ago

I recently had to look at src/sst/lf.cpp in order to figure out how SST's failure detection thread worked, and I was baffled by the huge number of function-like macros sprinkled throughout every function. These macros seem to contain a significant amount of functionality yet they are nearly impossible to read because of their formatting (e.g. the superfluous do { } while (0) loops and backslashes required for multiline macros, and the fact that macros can't be syntax-highlighted). Specifically, I'm referring to the macros FAIL_IF_NONZERO_RETRY_EAGAIN, FAIL_IF_ZERO, and CRASH_WITH_MESSAGE, which seem to implement error-handling code for libfabrics library functions, an important part of failure detection.

Since this is C++, not C, I don't see any reason to implement a function as a macro unless it needs to be completely compiled out in production, and these macros should never be compiled out in production (we always need to detect and handle errors from LibFabrics). They should be replaced with ordinary function calls, and the functions can be declared inline if we want to avoid the overhead of a stack frame when they are called.

songweijia commented 4 years ago

The libfabric itself is in C. And that's the way how many other applications using it. I agree that changing them to inline functions is an easy way to help you read it in Eclipse. There are not too many macros, you can go ahead to change them. Or do you want me to do that (in which branch)?

etremel commented 4 years ago

I'm happy to make the change myself, but it's good to hear that you agree with it. I'll do it in a small branch off of master, just in case I break something.