Skip to content

Do not call disabled deferred procedures #5183

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
May 20, 2025

Conversation

Feoramund
Copy link
Contributor

Fixes #5083

I put the warning token on the proc with the deferral as it's the first step to the cause of the issue, but I can see a case for either proc as being a valid warning source.

@Kelimion
Copy link
Member

Kelimion commented May 20, 2025

I don't know if it should print a warning, because then -warnings-as-errors will kill your build instead of fixing the bug. All it should probably do is wipe out the deferred call if said target call is disabled, even though the code is correct.

I'm one of those people who always compiles with odin build . -vet -vet-tabs -strict-style -vet-style -warnings-as-errors -disallow-do. 😂

@Feoramund
Copy link
Contributor Author

I can toss that out. I made it a warning because that's what I did when I fixed disabled @(init). Should that also not be a warning? My intent was consistency.

@Kelimion
Copy link
Member

Kelimion commented May 20, 2025

I can toss that out. I made it a warning because that's what I did when I fixed disabled @(init). Should that also not be a warning? My intent was consistency.

That's more likely to be an unintentional error. You run things under @(init) for a reason. If you want to disable it, you could also add a when DISABLED_REASON { return } as the first thing in those rare few exceptions where you don't want the warning, but also for that particular init to not do anything.

@Kelimion
Copy link
Member

(These things are hard, aren't they, deciding sane defaults? But we can always revisit these decisions if we get feedback on these particular obscure use cases.)

@Feoramund
Copy link
Contributor Author

(These things are hard, aren't they, deciding sane defaults? But we can always revisit these decisions if we get feedback on these particular obscure use cases.)

Yeah, I have a tendency to go for caution and informing the user as much as possible, but I can also see this being a less annoying result if it's commonplace for someone to have disabled deferrals for some reason.

@Kelimion
Copy link
Member

That's twice now that github's CDN failed:

Downloading PNG assets
	Downloading https://raw.githubusercontent.com/odin-lang/test-assets/master/PNG/PNG.zip to /home/runner/work/Odin/Odin/tests/core/assets/PNG/PNG.zip
Could not download https://raw.githubusercontent.com/odin-lang/test-assets/master/PNG/PNG.zip
Could not download ZIP file
/home/runner/work/Odin/Odin/tests/core/normal.odin([8](https://github.com/odin-lang/Odin/actions/runs/15126321729/job/42518940421?pr=5183#step:15:9):3) panic: downloading test assets failed!

@Feoramund
Copy link
Contributor Author

I've had issues with downloading the test assets locally today too.

@Kelimion
Copy link
Member

Kelimion commented May 20, 2025

(These things are hard, aren't they, deciding sane defaults? But we can always revisit these decisions if we get feedback on these particular obscure use cases.)

Yeah, I have a tendency to go for caution and informing the user as much as possible, but I can also see this being a less annoying result if it's commonplace for someone to have disabled deferrals for some reason.

I can see wanting to be cautious, but I also figure that if you're going out of your way to use deferred and disabled together, you're telling the compiler you at least think you know what you're doing. Neither is a beginner idiom, let alone together.

@Kelimion
Copy link
Member

And FreeBSD caps it off with a "VM is booting" loop. I think I'll merge the patch. :-)

@Kelimion Kelimion merged commit 50fcbb1 into odin-lang:master May 20, 2025
5 of 7 checks passed
@Kelimion
Copy link
Member

Confirmed the fix works locally. Thanks as always, Feoramund.

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.

@(disabled=!ODIN_DEBUG) doesn't work if procedure called using @(deferred_*=foo_debug)
2 participants