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

Minor cmake code duplication improvement #1283

Merged

Conversation

bencsikandrei
Copy link
Contributor

Create a macro to add tests for both static/header only.

The only differences between these two are the libraries they link
with and the target names.
There might be a reason to keep both, but it seemed like a nice opportunity
for a refactor.

Created the simple macro:

_spdlog_prepare_test( <spdlog_lib>)

which does the work.

Signed-off-by: Andrei-Florin BENCSIK [email protected]

Create a macro to add tests for both static/header only.

The only differneces between these two are the libraries they link
with and the target names. Created the simple macro:

_spdlog_prepare_test(<target> <spdlog_lib>)

which does the work.

Signed-off-by: Andrei-Florin BENCSIK <[email protected]>
@gabime
Copy link
Owner

gabime commented Oct 24, 2019

Thanks. what’s the advantage of macro here instead of a regular function?

@bencsikandrei
Copy link
Contributor Author

Hello,

In this case I think there isn't much of a difference since it's not setting anything inside the macro/function nor does it return() anything.
My understanding is that macros don't create a new scope, so all variables are string replaced (much like in C++) when the macro is called - also anything created inside is visible outside the scope, unlike functions which create a new scope. The return() statement inside a function returns from that particular function whilst from a macro it returns from the parent scope (as in C++, kind of).

I don't think there's much of an advantage one way or the other, they should work the same, but if you prefer a function I'm on board with that and I'll happily modify it.

@gabime
Copy link
Owner

gabime commented Oct 25, 2019

Since the rest of spdlog’s cmake scripts are already using functions, lets use a function here as well for the sake of consistency.

@bencsikandrei
Copy link
Contributor Author

Done!
I made another commit, but I believe it should be squashed from github (with the commit message of your choice).
Otherwise I can do it myself and rephrase the messages as well.

@gabime
Copy link
Owner

gabime commented Oct 25, 2019

It doesn't pass the tests unfortunately.

Be more consistent with the existing code and with the naming
_function -> function
@bencsikandrei bencsikandrei force-pushed the refactor/cmake-minor-duplication-removal branch from 4d8621d to 2cc620e Compare October 25, 2019 08:15
@bencsikandrei
Copy link
Contributor Author

Because VIM ate my 'end' from 'endfunction' :(

@gabime gabime merged commit d1dadc9 into gabime:v1.x Oct 25, 2019
bachittle pushed a commit to bachittle/spdlog that referenced this pull request Dec 22, 2022
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

Successfully merging this pull request may close these issues.

2 participants