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

Ensure LOG* and CHECK* macros are statement-safe #320

Merged
merged 1 commit into from
Jul 28, 2019

Conversation

madscientist
Copy link
Contributor

The LOG and LOGF macros had been modified to be statement-safe, but the
LOG_IF, CHECK, LOGF_IF, CHECKF, and CHECK_F macros were all still
unsafe in face of code where single-statement blocks were not enclosed
in {}.

For example code like this:

  if (!foobar)
      CHECKF(goodness, "badness detected!");
  else
      handle_foobar(foobar);

would fail in subtle and possibly dangerous ways.

Fix this by combining multiple if-statements into a single conditional
and inverting the conditions, then adding an empty then-block and moving
the log statement to the else-block.

The LOG and LOGF macros had been modified to be statement-safe, but the
LOG_IF, CHECK, LOGF_IF, CHECKF, and CHECK_F macros were all still
unsafe in face of code where single-statement blocks were not enclosed
in {}.

For example code like this:

  if (!foobar)
      CHECKF(goodness, "badness detected!");
  else
      handle_foobar(foobar);

would fail in subtle and possibly dangerous ways.

Fix this by combining multiple if-statements into a single conditional
and inverting the conditions, then adding an empty then-block and moving
the log statement to the else-block.
@KjellKod
Copy link
Owner

Looks good. Merge is pending testing.

Copy link
Owner

@KjellKod KjellKod left a comment

Choose a reason for hiding this comment

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

Nice work, thanks!

@KjellKod KjellKod merged commit f149179 into KjellKod:master Jul 28, 2019
@madscientist madscientist deleted the pds/safe-macros branch July 28, 2019 23:35
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