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

Fixup bitshift issue on 32-bit #1143

Merged
merged 1 commit into from
Feb 6, 2024
Merged

Conversation

nileshpatra
Copy link
Contributor

Currently nmodl fails to compile on 32-bit machines with:

/<<PKGBUILDDIR>>/src/symtab/symbol_properties.hpp:217:17: warning: left shift count >= width of type [-Wshift-count-overflow]
  217 |     define = 1L << 32,
      |              ~~~^~~~~
/<<PKGBUILDDIR>>/src/symtab/symbol_properties.hpp:217:17: error: right operand of shift expression ‘(1 << 32)’ is greater than or equal to the precision 32 of the left operand [-fpermissive]
/<<PKGBUILDDIR>>/src/symtab/symbol_properties.hpp:217:20: error: enumerator value for ‘define’ is not an integer constant
  217 |     define = 1L << 32,
      |                    ^~
/<<PKGBUILDDIR>>/src/symtab/symbol_properties.hpp:220:22: warning: left shift count >= width of type [-Wshift-count-overflow]
  220 |     codegen_var = 1L << 33
      |                   ~~~^~~~~
/<<PKGBUILDDIR>>/src/symtab/symbol_properties.hpp:220:22: error: right operand of shift expression ‘(1 << 33)’ is greater than or equal to the precision 32 of the left operand [-fpermissive]
/<<PKGBUILDDIR>>/src/symtab/symbol_properties.hpp:220:25: error: enumerator value for ‘codegen_var’ is not an integer constant
  220 |     codegen_var = 1L << 33
      |                         ^~

This is an attempt to fix the same. Verified that it builds on a 32-bit chroot on v0.6.

/cc: @pramodk

@alkino
Copy link
Member

alkino commented Jan 27, 2024

As the underlying type is long long maybe you can use LL everywhere

Copy link

codecov bot commented Jan 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5abaad3) 88.22% compared to head (e1317f6) 88.22%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1143   +/-   ##
=======================================
  Coverage   88.22%   88.22%           
=======================================
  Files         175      175           
  Lines       13021    13021           
=======================================
  Hits        11488    11488           
  Misses       1533     1533           

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

@alkino
Copy link
Member

alkino commented Jan 27, 2024

Maybe uLL or uint64_t would be better?

@pramodk
Copy link
Contributor

pramodk commented Jan 27, 2024

As the underlying type is long long maybe you can use LL everywhere

➕ (i.e. in NmodlType). Otherwise, LGTM!

thanks @nileshpatra !

@nileshpatra
Copy link
Contributor Author

As the underlying type is long long maybe you can use LL everywhere

➕ (i.e. in NmodlType). Otherwise, LGTM!

There are other interfaces as well apart fro NmodlType that use enum_type = long long. I've added LL for all such blocks. Kindly review.

@pramodk
Copy link
Contributor

pramodk commented Jan 27, 2024

Maybe uLL or uint64_t would be better?

@alkino : ULL should be fine (/better?) as well. But as LL and ULL would typically be at least 64 bits, this should be Ok as well, right?

@alkino
Copy link
Member

alkino commented Jan 27, 2024

Right. I only find it more clear if this is U. I would advocate for ULL
Keep it like that. We can make the move in a following PR

@alkino alkino enabled auto-merge (squash) January 27, 2024 14:31
@nileshpatra
Copy link
Contributor Author

The Sonar CI fails with:

ERROR: Project not found. Please check the 'sonar.projectKey' and 'sonar.organization' properties, the 'SONAR_TOKEN' environment variable, or contact the project administrator

I don't think I can do anything here. Can you please do the needful?

@alkino alkino disabled auto-merge January 28, 2024 18:40
@alkino alkino enabled auto-merge (squash) January 28, 2024 18:40
@alkino
Copy link
Member

alkino commented Jan 28, 2024

The Sonar CI fails with:

ERROR: Project not found. Please check the 'sonar.projectKey' and 'sonar.organization' properties, the 'SONAR_TOKEN' environment variable, or contact the project administrator

I don't think I can do anything here. Can you please do the needful?

We take care of that, thank you for your PR. It will be merge soon or later (tomorrow).

@ohm314
Copy link
Contributor

ohm314 commented Jan 30, 2024

@nileshpatra, I think if you rebase your PR branch on our latest master the sonarcloud job should become non-failing.

@alkino alkino disabled auto-merge January 30, 2024 12:26
@nileshpatra
Copy link
Contributor Author

@nileshpatra, I think if you rebase your PR branch on our latest master the sonarcloud job should become non-failing.

Thanks it seems green now. The remaining (docs/epfl CI) probably need your approval?

@alkino alkino enabled auto-merge (squash) January 30, 2024 23:25
@alkino alkino closed this Jan 31, 2024
auto-merge was automatically disabled January 31, 2024 08:24

Pull request was closed

@alkino alkino reopened this Jan 31, 2024
@alkino alkino enabled auto-merge (squash) January 31, 2024 08:24
@alkino alkino closed this Feb 5, 2024
auto-merge was automatically disabled February 5, 2024 09:41

Pull request was closed

@alkino alkino reopened this Feb 5, 2024
@alkino alkino enabled auto-merge (squash) February 5, 2024 09:41
@pramodk pramodk closed this Feb 5, 2024
auto-merge was automatically disabled February 5, 2024 14:50

Pull request was closed

@pramodk pramodk reopened this Feb 5, 2024
@pramodk pramodk merged commit 3e3f62a into BlueBrain:master Feb 6, 2024
51 of 52 checks passed
ohm314 pushed a commit that referenced this pull request May 21, 2024
@nileshpatra nileshpatra deleted the 32-bit branch September 11, 2024 19:08
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.

4 participants