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

Fix stoi error when passing --jobs=auto in non-OpenMP code path #2477

Merged

Conversation

strRM
Copy link
Contributor

@strRM strRM commented Mar 15, 2024

When passing auto to the jobs in non-OpenMP mode, the "auto" ends up as an argument to std::stoi in a couple of places, causing the program to crash. We fix this by mapping auto to 1 when OpenMP is not available.

Fixes #2475

Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.79%. Comparing base (d804988) to head (f58f4c2).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2477      +/-   ##
==========================================
- Coverage   77.79%   77.79%   -0.01%     
==========================================
  Files         492      492              
  Lines       33188    33189       +1     
==========================================
  Hits        25818    25818              
- Misses       7370     7371       +1     
Files Coverage Δ
src/MainDriver.cpp 70.55% <100.00%> (+0.05%) ⬆️

... and 2 files with indirect coverage changes

Copy link
Member

@quentin quentin left a comment

Choose a reason for hiding this comment

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

The openmp counterpart has an explicit test for for number vs auto and throws a runtime error for other cases. Should we repurpose/reuse that code?

@strRM
Copy link
Contributor Author

strRM commented Mar 15, 2024

Yeah, let me rewrite this a little bit.

When passing `auto` to the jobs in non-OpenMP mode, the `"auto"` ends
up as an argument to `std::stoi` in  a couple of places, causing the
program to crash. We fix this by mapping `auto` to `1` when OpenMP
is not available.

Fixes souffle-lang#2475
@strRM strRM force-pushed the rm/cet-2475-fix-stoi-no-conversion branch from 0ee65f8 to f58f4c2 Compare March 15, 2024 16:20
@strRM
Copy link
Contributor Author

strRM commented Mar 15, 2024

I don't like the 2 ifdefs. I suppose I could introduce a boolean variable haveOpenMP and then use that instead the ifdefs.

@strRM
Copy link
Contributor Author

strRM commented Mar 16, 2024

I assume you're aware of the failures due to running out of disk space?

@quentin quentin merged commit d3661e5 into souffle-lang:master Mar 16, 2024
25 of 30 checks 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.

no conversion for stoi when parsing --jobs=auto (without OPENMP)
3 participants