RGLab / cytolib

c++ library for representing and interacting the gated cytometry data structure
GNU Affero General Public License v3.0
12 stars 11 forks source link

eliminate global variables #3

Open mikejiang opened 6 years ago

mikejiang commented 6 years ago

There are currently two global variables used by cytolib (mainly for troubleshooting purpose) https://github.com/RGLab/cytolib/blob/trunk/inst/include/cytolib/global.hpp#L30-L31

extern keyword indicates they are only the variable declaration instead of definition. Because due to the nature of header-only, cytolib can't define them in any header in order to avoid multi-definition compiling errors (when header is included multiple times).

Therefore it implicitly requires the cytolib users to define it somewhere in the user code in order for cytolib function, even if they don't necessarily need to care about this log feature.

We shouldn't force user to do this (besides it is not best practice to use global variable). Instead, we can move them into GatingSet class and initialize/change their values either through parseWorkspace call or through setter methods once GatingSet is created.

mikejiang commented 6 years ago

It turns out to be a lot of hassles to initialize and pass around the local version of this loglevel flag due to its universal usage among every classes defined in cytolib. We will need to figure out more elegant way to handle this (maybe the third-party logging lib).

gfinak commented 6 years ago

Maybe a naive question, but wouldn't a conditional definition using the preprocessor work?

mikejiang commented 6 years ago

I am not sure if that will allow user to switch on/off the log at runtime

mikejiang commented 6 years ago

For now, I defined a macro as CYTOLIB_INIT() to make it more user friendly, see https://github.com/RGLab/flowWorkspace/blob/trunk/src/R_GatingSet.cpp#L13

mikejiang commented 6 years ago

@gfinak , #ifdef only prevents the same source file(i.e. single translation unit) from defining it multiple times, but different source files can still include the same header independently, which will pass the compilation, and eventually causing linking error complaining the conflicting definitions among these translation units.

gfinak commented 6 years ago

Yes, makes sense. Thanks for the clarification.