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

✨ Allow #embed file replacement and preserve eol-tokens #5600

Conversation

ThePhD
Copy link
Contributor

@ThePhD ThePhD commented Oct 14, 2023

Text that comes after the #embed (or #include) is also now preserved, as in previous times they were ignored (this was allowed by both standards for #include, but the tokens that come after #embed do matter as they are parameters).

@github-actions github-actions bot added the ui label Oct 14, 2023
@partouf
Copy link
Member

partouf commented Oct 14, 2023

This is great! When I refactored the #include code, I did think about adding #embed, but it would've resulted in a messy PR at the time and then I forgot about it. So thanks!

@ThePhD
Copy link
Contributor Author

ThePhD commented Oct 14, 2023

Well, good to hear I got to clean this up for you from the old idea!

Anything I need to do in other places? I only found that I had to modify here through brute-force searching for things to change; any other places that need the regex handling changes?

Text that comes after the `#embed` (or `#include`) is also now preserved, as in previous times they were ignored (this was allowed by both standards for `#include`, but the tokens that come after `#embed` do matter as they are parameters).
@ThePhD ThePhD force-pushed the thephd/embed-parameter-and-https-support branch from f8e5b23 to 441e238 Compare October 14, 2023 22:58
@partouf
Copy link
Member

partouf commented Oct 14, 2023

Well, good to hear I got to clean this up for you from the old idea!

Anything I need to do in other places? I only found that I had to modify here through brute-force searching for things to change; any other places that need the regex handling changes?

I don't think so, I think it's all in here. But we'll give it a test or two

@partouf
Copy link
Member

partouf commented Oct 21, 2023

Looks to be working correctly in various situations.

Although I was unable to find a compiler that supports this yet, do you know of any?

@ThePhD
Copy link
Contributor Author

ThePhD commented Oct 21, 2023

You can use the dang branch (the thephd.dev one). I updated it to have the newest #embed. Patch is out for Clang, but it's a sufficiently complex monster that it'll be a while:

llvm/llvm-project#68620
https://thephd.dev/implementing-embed-c-and-c++

@ThePhD
Copy link
Contributor Author

ThePhD commented Oct 21, 2023

Sorry, forgot to include a quick link: https://godbolt.org/z/Pb35fW7qs

@partouf
Copy link
Member

partouf commented Oct 21, 2023

Ahhhh, thanks. I did try that, but with c++ and then I got a warning with the example that's on https://en.cppreference.com/w/c/preprocessor/embed

/app/example.cpp:3:79: warning: unknown embed preprocessor parameter 'if_empty' ignored [-Wunknown-directive-parameters]
#embed "raw.githubusercontent.com/partouf/ManaWarning/master/ManaWarning.lua" if_empty('M', 'i', 's', 's', 'i', 'n', 'g', '\n')

but that's of course just about the if_empty and not the embed itself

Ok, cool

@partouf partouf merged commit c6d3fa6 into compiler-explorer:main Oct 21, 2023
8 checks passed
@partouf
Copy link
Member

partouf commented Oct 21, 2023

Also I do not know how well this will work with binary files. After this goes live and you have an example that goes awry, let me know

@ThePhD
Copy link
Contributor Author

ThePhD commented Oct 21, 2023

Weird, I must've goofed something up. But, thanks for merging! Here's to waiting 24 hours to see it hit the main site. 🎉

@partouf
Copy link
Member

partouf commented Oct 22, 2023

this is now live

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

Successfully merging this pull request may close these issues.

2 participants