Skip to content

Handle matters of trivial syntax #177

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

Merged
merged 1 commit into from
Jun 9, 2025

Conversation

ckormanyos
Copy link
Contributor

@ckormanyos ckormanyos commented Jun 8, 2025

Hi Matt (@mborland)

I really appreciate your work on cppallicnace/int128. It is looking really good so far.

The purpose of this PR is to address matters of trivial syntax. It involves 3 main things.

  • The changes in traits.hpp address an extra semicolon found with -Wpedantic. You don't have this warning in CI, probably on purpose. This was the only -Wpedantic warning that I found in my brief trials.
  • Shield (numeric_limits<T>::min)() and (numeric_limits<T>::max)() from the MSVC <windows.h> header which actually #defines macros for min() and max(). This is a standard boost workaround and there is even a CI style checker that fails CI if the shielding parentheses are absent.
  • Sync up some of the header guard definitions with the actual file names. I'm not sure if you'll want these changes, but they seemed to make sense to me.

There is a fourth item I observed. In Knuth long division here, there is a right-shift of signed int64_t. This requires an arithmetic right shift. In the old days, signed right shift was not guaranteed to properly handle the sign-bit (although I never encountered a compiler that did not properly do arithmetic right shift). It is, however, I believe worth pointint out. I know that is a speed-critical routine, so it's maybe in the area of don't care?

Copy link

codecov bot commented Jun 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.92%. Comparing base (2d513a2) to head (11fc059).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #177   +/-   ##
=======================================
  Coverage   98.92%   98.92%           
=======================================
  Files          33       33           
  Lines        4186     4186           
  Branches     1069     1069           
=======================================
  Hits         4141     4141           
  Misses         41       41           
  Partials        4        4           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@mborland mborland left a comment

Choose a reason for hiding this comment

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

This all looks good to me. Thanks Chris!

@mborland mborland merged commit fd899d6 into cppalliance:master Jun 9, 2025
73 checks passed
@ckormanyos ckormanyos deleted the trivial_syntax branch June 9, 2025 19:17
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