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

[superseded] fix #12996 #12997

Closed
wants to merge 1 commit into from

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Dec 31, 2019

# Pragmas for RTL generation. Has to be an include, because user-defined
# pragmas cannot be exported.

@Araq
Copy link
Member

Araq commented Dec 31, 2019

I understand the desire to put it into system.nim, but I don't understand #12996 yet so please be a little patient. :-)

@timotheecour
Copy link
Member Author

The bug is simple: include inclrtl is only enabled for non js target, so since is not visible in js mode, hence the regression

@Araq
Copy link
Member

Araq commented Dec 31, 2019

Why not include it all the time then?

@timotheecour
Copy link
Member Author

well that's the alternative i had written up in top post:

the alternative is to add include "system/inclrtl" (even for nim js) to strtabs.nim but IMO since does not belong in inclrtl:

but as i mentioned, it's not so good to include inclrtl especially since it has nothing to do with inclrtl (and include is a code smell IMO).

related question: I never understood the rationale for : pragmas cannot be exported
is there a fundamental reason for this limitation or it's just a matter of someone implementing export pragmas?

@Araq
Copy link
Member

Araq commented Jan 1, 2020

Well since is for Nim's stdlib, not for anybody else and so it shouldn't be exported from system.nim. The stdlib should include what it needs instead, JS target or not.

I never understood the rationale for : pragmas cannot be exported
is there a fundamental reason for this limitation or it's just a matter of someone implementing export pragmas?

Looks somewhat hard to implement but more importantly, I considered {.pragma.} a hack until templates pragmas worked. And now they do work.

@timotheecour
Copy link
Member Author

timotheecour commented Jan 3, 2020

Well since is for Nim's stdlib, not for anybody else and so it shouldn't be exported from system.nim. The stdlib should include what it needs instead, JS target or not.

good point regarding not making since public; this is actually a perfect example of the need for {. privateImport.} (see #11865 which I just rebased).

As a concrete example, see how this would play out here with since added privately to system.nim: timotheecour@643a326

# in strtabs.nim:
from system {.privateImport.} import since

besides removing most needs for include, making testing easier and other benefits I mentioned in top post of that PR, this would actually help un-bloat system.nim in similar ways.

@Araq
Copy link
Member

Araq commented Jan 5, 2020

I fixed it differently.

@Araq Araq closed this Jan 5, 2020
@timotheecour timotheecour deleted the pr_fix_12996_since branch June 16, 2020 23:44
@timotheecour timotheecour changed the title fix #12996 [superseded] fix #12996 Jun 19, 2020
@timotheecour
Copy link
Member Author

superseded by #14188

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.

regression(1.04) invalid pragma: since with nim js
2 participants