Improve Macro Safety and Casting Practices: remove trailing semicolon

[anandamideShakyan]

PR Description:

This pull request addresses two macro-related issues found in velox/tpch/gen/dbgen/build.cpp and velox/common/base/SpillStats.cpp to improve code correctness and maintainability:

1. Add Parentheses Around Macro Parameters (velox/tpch/gen/dbgen/build.cpp)

Issue: The TEXT macro uses parameters like avg without parentheses, which can lead to incorrect results if the parameter is an expression. For example:

#define TEXT(avg, seed, tgt) \
  dbg_text(tgt, (int)(avg * V_STR_LOW), (int)(avg * V_STR_HGH), seed)

If avg is replaced with an expression (e.g., a + b), the result would be incorrect due to operator precedence:

TEXT(a + b, seed, tgt) // Expands to dbg_text(tgt, (int)(a + b * V_STR_LOW), ...)

Fix: Wrap macro parameters in parentheses to ensure correct precedence and behavior:

#define TEXT(avg, seed, tgt) \
  dbg_text(tgt, static_cast<int>((avg) * V_STR_LOW), static_cast<int>((avg) * V_STR_HGH), seed)

We are also replacing (int) C style casts with a static_cast as it ensures safer, clearer, and more maintainable type conversions by enforcing explicit and compile-time-checked behavior.

2. Remove Trailing Semicolon in Macro Definition (velox/common/base/SpillStats.cpp)

Issue: The UPDATE_COUNTER macro ends with a semicolon after the while (0) block:

#define UPDATE_COUNTER(counter)           \
  do {                                    \
    if (counter < other.counter) {        \
      ++ltCount;                          \
    } else if (counter > other.counter) { \
      ++gtCount;                          \
    }                                     \
  } while (0);

This results in double semicolons (;;) when the macro is used:

UPDATE_COUNTER(someCounter); // Expands to: do { ... } while (0);;

Fix: Remove the trailing semicolon in the macro definition:

#define UPDATE_COUNTER(counter)           \
  do {                                    \
    if ((counter) < (other.counter)) {    \
      ++ltCount;                          \
    } else if ((counter) > (other.counter)) { \
      ++gtCount;                          \
    }                                     \
  } while (0)

These fixes enhance code safety and eliminate potential pitfalls in macro usage.

[facebook-github-bot]

Hi @anandamideShakyan!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[netlify]

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	186f320
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/67610fc874daf20009c9a395

[czentgr]

Please check if the casts are already addressed by the PRs that fix the C-syle casts (see open PRs for those).

Please also fix the prior macro that has the same issue for the avg argument.

[anandamideShakyan]

Fixed the prior macro. I checked and some c style casts were resolved but these casts were not addressed.

[jkhaliqi]

I believe this file changes can be removed since it looks like it has been done in this PR
https://github.com/facebookincubator/velox/pull/11688/files

[czentgr]

@anandamideShakyan Please provide a description (see guidelines in contributing).
For example, https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es41-if-in-doubt-about-operator-precedence-parenthesize can e used as rationale when expanding macros.

In the description also mention cleanup of semicolons.

Also please squash commits and provide a better abstract than copying the name from the internal issue.

[jkhaliqi]

I believe this file changes can be removed since it looks like it has been done in this PR
https://github.com/facebookincubator/velox/pull/11688/files

[anandamideShakyan]

@jkhaliqi done.