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

Fixes bug with variable naming in a couple of macros #596

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

ilumsden
Copy link
Contributor

This PR fixes a bug with two Caliper macros:

  • CALI_CXX_MARK_FUNCTION
  • CALI_CXX_MARK_SCOPE

Both of these macros creates variables using special macros (i.e., __func__ and __LINE__) and token concatenation (i.e., ##). However, token concatenation prevents recursive evaluation of macros, which can cause compilation issues.

As an example, consider the following code:

CALI_CXX_MARK_SCOPE("outer_region");
CALI_CXX_MARK_SCOPE("inner_region");

After pre-processing, the code would look like:

cali::ScopeAnnotation __cali_ann_scope__LINE__("outer_region");
cali::ScopeAnnotation __cali_ann_scope__LINE__("inner_region");

Since these two variables have the same name, this code will fail to compile.

This PR fixes this bug by making use of the macro logic from this StackOverlow post.

// These macros were obtained from:
// https://stackoverflow.com/a/71899854
#define CONCAT_(prefix, suffix) perfix##suffix
#define CREATE_VAR_NAME(prefix, suffix) CONCAT_(prefix, suffix)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefix these with CALI_ just to be sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I'll do that. Also, I'll tweak the logic for checking the results of the unit tests. I didn't realize that my test change would mess with the result logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

d8055c6 resolves this and fixes unit tests.

@daboehme daboehme merged commit 1e8315b into LLNL:master Sep 30, 2024
1 check passed
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