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

Add read_file macro method #2791

Closed
wants to merge 1 commit into from
Closed

Conversation

asterite
Copy link
Member

@asterite asterite commented Jun 10, 2016

Check the diff for the docs.

The rationale for introducing this is that:

  1. Embedding resources has already popped up several times, either here in github or talking with others personally.
  2. One can currently use cat filename.txt but that's not portable, and one has to redirect errors to avoid getting a compilation error.
  3. read_file returns nil if the file doesn't exist. With that, one can choose to deal with nil at compile time or runtime.
  4. It should be a bit faster because an external process doesn't need to be started

This works for text files, but one can also use it for binary data: simply call to_slice afterwards to get the bytes (though we probably need a literal for binary data, but that's another issue)

Actually, it doesn't work for binary files because the string won't be able to be embedded at all. We could probably add a read_binary_file later, if we really want to do that.

Fixes #1649 and #2751

@asterite
Copy link
Member Author

Forgot to mention that this works more or less like the system call: one has to use __DIR__ to get a file relative to the file where the macro is defined. But I think that's OK.

@asterite
Copy link
Member Author

Nim is another language that has this feature: https://github.com/nim-lang/Nim/blob/master/lib/system.nim#L3262 , and the reason is embedding resources too.

@jhass
Copy link
Member

jhass commented Jun 10, 2016

To be honest I'm not sure we should do this before it works for any kind of data. Returning Slice(UInt8) should be the general case, not the special case. Either variant should return a pointer into the data section of the generated binary, so I don't think this can be a macro that just returns any kind of literal.

We currently don't support a platform where the cat workaround doesn't work, so I'm not sure this improves much at all.

@jhass
Copy link
Member

jhass commented Jun 10, 2016

https://github.com/graphitemaster/incbin shows an interesting approach, but sadly LLVM says no:

asm(%(
.section .rodata
.global gBlobData
.type gBlobData, @object
.balign 16
gBlobData:
.incbin "hello.cr"
.global gBlobEnd
.type gBlobEnd, @object
.balign 1
gBlobEnd:
.byte 1
.global gBlobSize
.type gBlobSize, @object
.balign 16
gBlobSize:
.int gBlobEnd - gBlobData
))

lib LibEmbed
  $gBlobData : LibC::Char*
  $gBlobSize : LibC::UInt
end

pp LibEmbed.gBlobSize
pp LibEmbed.gBlobData
blob = Slice.new(LibEmbed.gBlobData, 1)
pp blob
LLVM ERROR: Cannot represent a difference across sections

@faultyserver faultyserver mentioned this pull request May 24, 2017
@Sija
Copy link
Contributor

Sija commented May 24, 2017

Bump.

@asterite
Copy link
Member Author

Just use {{ `cat file.txt` }}

@asterite asterite closed this May 24, 2017
@oprypin
Copy link
Member

oprypin commented May 24, 2017

Great preparation for Windows support.

@Sija
Copy link
Contributor

Sija commented May 24, 2017

@asterite And what about reasons you've given in point no. 2? Also, I'd say that closing PR with 6 x
👍 without any discussion is a bit... hasty.

@asterite
Copy link
Member Author

@Sija With ifdef one can create a version for Windows if that matters. Adding this makes the language more complex, and it feels like a hack (we can't keep adding macro methods forever)

@Sija
Copy link
Contributor

Sija commented May 24, 2017

@asterite I think you meant if flag?(:windows), ifdef was removed in v0.20.0. Personally I think that your proposal seems more hack-ish than adding this - quite requested - macro method.

@woodruffw
Copy link
Contributor

Any chance of this or something similar being revived? It'd be nice to have a platform independent/shell-free way to embed data in a macro.

@straight-shoota
Copy link
Member

You might want to take a look at schovi/baked_file_system.

@woodruffw
Copy link
Contributor

Thanks @straight-shoota! I saw that in #1649 too. I'd love to see this become part of the language at some point, but baked_file_system is good for now 😄

@asterite asterite deleted the feature/macro_read_file branch April 27, 2018 21:19
@Sija Sija mentioned this pull request Sep 24, 2018
@Sija
Copy link
Contributor

Sija commented Sep 24, 2018

Could this PR be revaluated once more? @asterite come to your senses, please! ;)

@RX14
Copy link
Contributor

RX14 commented Sep 25, 2018

@jhass the reason LLVM will complain is that asm() at the top level is still inline asm in that it's inside a method. This should be doable if global/module level ASM was implemented.

@jhass
Copy link
Member

jhass commented Sep 25, 2018

Cool, so let's do that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants