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

Expose alt_name for redefine_main #4798

Closed

Conversation

Fryguy
Copy link
Contributor

@Fryguy Fryguy commented Aug 6, 2017

This changes redefine_main to expose the alternate name as a parameter. The purpose of this is to allow an author to create the crystal main function without literally naming it main.

In my case, as described in #4793, I am writing a binding to Allegro. Allegro requires that you pass your "main" function to theirs as a callback. So, I need to pass Crystal's main function to Allegro's main function handler, which means I need to get a reference to it. With the change in this PR, I can avoid copy-pasting all of redefine_main just to avoid the fun main = part.

I had debated renaming redefine_main to define_main, and writing a separate redefine_main that would call define_main, but I couldn't seem to embed one macro in another without getting a unterminated macro error.

Closes #4793

Fryguy added a commit to Fryguy/crystal_allegro that referenced this pull request Aug 6, 2017
Previously, Crystal owned the main function, then called Allegro's
al_run_main with a function pointer to a Crystal method, but this would
cause segfaults on Fiber switches.  Now, we make Allegro own the main
function, and pass Crystal's main function to it instead, which works as
expected.

We are keeping a slightly modified copy of redefine_main until
crystal-lang/crystal#4798 is merged.
@RX14
Copy link
Contributor

RX14 commented Sep 10, 2017

I'm actually a little confused here, what are the two names for in a fun with body and why do you need both?

@asterite
Copy link
Member

I think the issue is that redefine_main by default defines a function that's called, for LLVM and C, whatever you want, but in Crystal it's always called "main". So if you want to redefine main to call that redefined main you have no way, because the name "main" is already taken. This PR lets you change the Crystal name.

Maybe there's a more elegant way to solve this?

@mverzilli
Copy link

mverzilli commented Sep 16, 2017

I think the solution is pretty elegant, but we'd need at least some basic documentation for this, otherwise this will be an obscure feature and no one will know about it.

@asterite
Copy link
Member

The use case is pretty obscure already. Allegro requires main to invoke their main, so main needs to be redefined. We can document it, but very few bindings will actually need this. 99.999% of the apps don't need to redefine main.

@mverzilli
Copy link

I beg to differ here. If the feature is worth adding, it is worth documenting. The fact that it is obscure and rarely used actually is an argument in favor of documenting: I can get away with not documenting how if works because almost every programmer is acquainted with it and is likely to pick it up without help. But things like this cause FUD: "what is alt_name?", "should I use it?", "will I break something?", "what are valid alt_names?", etc.

IMO, we've got two options: either we accept it with documentation or we reject it. To be clear, by documentation I mean simply something along the lines of what @Fryguy put in the description of this PR, with a little more context, in the autogenerated docs of redefine_main.

@asterite
Copy link
Member

Sounds good.

@Fryguy Care to add some doc comments?

@asterite
Copy link
Member

Actually, I'm thinking this has a much simpler solution. Right now main is generated as:

fun main = main(...)
end

The name in the left side is the name used to invoke that function from Crystal. The name on the right is is the name to generate for C.

But, the name on the left side is never invoked in Crystal code. So we can always call it crystal_main, for example. Then you can redefine it without another name for C, but for Crystal it will always be called crystal_main. And you can do ->crystal_main if you need a reference to it.

In fact, main.cr can be refactored in a much simpler way. I'll send a PR soon.

@Sija
Copy link
Contributor

Sija commented Sep 21, 2017

Fixed by #4998.

@asterite asterite closed this Sep 21, 2017
@Fryguy Fryguy deleted the alt_name_for_redefine_main branch November 6, 2017 20:32
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.

None yet

5 participants