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

cgen: fix regression with .nodecl consts #785

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Jul 7, 2023

Summary

Fix a regression where the C code generator ignored the .nodecl
pragma for constants.

Details

Apart from the fix, the single-use genConstSetup is inlined into
useConst. Since the loc for constants is setup in genConstDefinition
(which is always called before useConst), the fillLoc call is not
added to useConst (this is also a step towards fillLoc only being
used in procedures responsible for definitions).

In addition, a test for the current behaviour of imported .nodecl

Summary
=======

Fix a regression where the `.nodecl` pragma was ignored by the C code
generator in the context of constants.

Details
=======

Apart from the fix, the single-use `genConstSetup` is inlined into
`useConst`. Since the loc for constants is setup in `genConstDefinition`
(which is always called before `useConst`), the `fillLoc` call is not
added to `useConst` (this is also a step towards `fillLoc` only being
used in procedures responsible for definitions).

In addition, a test for the current behaviour of imported `.nodecl`
constants is added.
@zerbina zerbina added bug Something isn't working compiler/backend Related to backend system of the compiler labels Jul 7, 2023
@zerbina zerbina added this to the C backend refactoring milestone Jul 7, 2023
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

Does fix the issue and it's good to have a regression test for it, as well.

Outside of the PR the behaviour difference between compile and runtime is a bit weird. Although I can't yet articulate what's off/missing.

@saem
Copy link
Collaborator

saem commented Jul 7, 2023

/merge

@github-actions
Copy link

github-actions bot commented Jul 7, 2023

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

@chore-runner chore-runner bot added this pull request to the merge queue Jul 7, 2023
Merged via the queue into nim-works:devel with commit 9545f36 Jul 7, 2023
@zerbina zerbina deleted the cgen-fix-const-regression branch July 8, 2023 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler/backend Related to backend system of the compiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants