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.

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

eliminate global variables #3

Open
mikejiang opened this issue Jan 12, 2018 · 6 comments
Open

eliminate global variables #3

mikejiang opened this issue Jan 12, 2018 · 6 comments

Comments

@mikejiang
Copy link
Member

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
Copy link
Member Author

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).

mikejiang pushed a commit to RGLab/flowWorkspace that referenced this issue Jan 13, 2018
@gfinak
Copy link
Member

gfinak commented Jan 13, 2018

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

@mikejiang
Copy link
Member Author

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

mikejiang pushed a commit that referenced this issue Jan 13, 2018
mikejiang pushed a commit that referenced this issue Jan 13, 2018
This reverts commit c0a713e.
mikejiang pushed a commit that referenced this issue Jan 13, 2018
mikejiang pushed a commit to RGLab/flowWorkspace that referenced this issue Jan 13, 2018
@mikejiang
Copy link
Member Author

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
Copy link
Member Author

@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
Copy link
Member

gfinak commented Jan 13, 2018

Yes, makes sense. Thanks for the clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants