FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add runtime check for modulo by zero by LT2H · Pull Request #1405 · hsutter/cppfront · GitHub

Add runtime check for modulo by zero - #1405

Merged
hsutter merged 2 commits into
hsutter:mainfrom
LT2H:modulo-by-zero-check
Jan 5, 2026
Merged

hsutter merged 2 commits into
hsutter:mainfrom
LT2H:modulo-by-zero-check

Conversation

LT2H commented Jun 10, 2025 •
edited by hsutter
Loading

Copy link
Copy Markdown
Contributor

Checks for modulo and modulo-assignment by zero at runtime. Based on a suggestion in #1399.
Resolves #1399.

LT2H force-pushed the modulo-by-zero-check branch from b391976 to f9442fe Compare August 12, 2025 06:18
One of the tests now hits an MSVC error I've seen before, related to using std::source_location via module std import -- it doesn't manifest on any other compiler, or in MSVC using std headers. So the error is spurious, but I'm not able to debug it or find a workaround, and I don't want to record regression test failures just because of that use case where 'module std currently doesn't work.' So this commit also changes all -pure regression tests to use headers, not modules, at least for now until modules work better

hsutter commented Jan 5, 2026

Copy link
Copy Markdown
Owner

Thanks!

hsutter merged commit 71a17ed into hsutter:main Jan 5, 2026
14 of 26 checks passed

gregmarr commented Jan 5, 2026

Copy link
Copy Markdown
Contributor

@hsutter Did you intend to switch from importing std to including std when running the tests?

hsutter commented Jan 6, 2026

Copy link
Copy Markdown
Owner

@hsutter Did you intend to switch from importing std to including std when running the tests?

That's probably a good idea. @jarzec was maintaining and updating the test runner (thanks again!) -- do those use MSVC modules today which would have required that modules be built somehow (I vaguely recall seeing something like that)? If they do they should probably also switch to use -in to include std headers instead of importing the std module, which is what I did in this commit.

jarzec commented Jan 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

@hsutter Did you intend to switch from importing std to including std when running the tests?

That's probably a good idea. @jarzec was maintaining and updating the test runner (thanks again!) -- do those use MSVC modules today which would have required that modules be built somehow (I vaguely recall seeing something like that)? If they do they should probably also switch to use -in to include std headers instead of importing the std module, which is what I did in this commit.

I have to admit I forgot the details, but clearly there was some effort at some stage to make import usable.
The tests were passing with imports (except the Windows runners where we have build issues, but that is a separate story).
I would indeed revert that particular change (not the whole PR) and switch back to imports.

@hsutter I will soon try open a quick PR readding imports. You can either merge it or close it.

First, however, I will update the regression tests to have a clean start.

EDIT
I have only just fully understood your response. I will update the tests shortly.

jarzec commented Jan 7, 2026

Copy link
Copy Markdown
Contributor

Regression tests are fixed (among other changes) in #1442

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

[SUGGESTION] Add runtime checks for C++ integer modulo by zero

4 participants


Back | FazBrowse Home | New Git URL