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

Partially split Blocks.xs into some .c/.h files? #19

Open
tsee opened this issue Jan 11, 2017 · 11 comments
Open

Partially split Blocks.xs into some .c/.h files? #19

tsee opened this issue Jan 11, 2017 · 11 comments

Comments

@tsee
Copy link
Contributor

tsee commented Jan 11, 2017

I've been wondering: Blocks.xs is pretty big at this point. How would you feel about splitting some of it out into their own .c/.h files to decouple things a bit (where possible)?

@tsee tsee mentioned this issue Jan 11, 2017
@tsee
Copy link
Contributor Author

tsee commented Jan 11, 2017

Closing this because superseded by #20.

@tsee tsee closed this as completed Jan 11, 2017
@run4flat
Copy link
Owner

Reopening until I finish extracting the parser code.

@run4flat run4flat reopened this Jan 12, 2017
@tsee
Copy link
Contributor Author

tsee commented Jan 12, 2017

Quoting your concern in the pull request.

Something I've always struggled with: do the compiled object files get linked to all compiled xs files? In other words, will this functionality get baked into PerlAPI's shared library? As far as I can tell, this is what Module::Build does by default.

That's a really good point and I'd never considered it. Mostly because I'd never actually gone and used multiple XS files => shared libs in the same CPAN distribution. I'd always just done INCLUDE: xs/other_file.xs to structure the code and then gotten a single .so file in the end.

I just checked the shared libs to see where the symbols from the extracted C end up:

~/perl/C-Blocks$ nm blib/arch/auto/C/Blocks/Blocks.so | grep cb_build_op
000000000000f9c0 T cb_build_op
~/perl/C-Blocks$ nm blib/arch/auto/C/Blocks/PerlAPI/PerlAPI.so | grep cb_build_op
000000000003b0f0 T cb_build_op

Alas, it's in both. :(

The linker commands show the same thing, of course, listing all the .o's.

Sorry for not realizing that before. I don't know if there's a Module::Build-friendly way of getting it to do the right thing. My fear there might not be. One can, of course, always subclass the hell out of it and manually insert special casing logic. That might be the least worst option. :( (The other option I can see is to simply do much of the compiling linking by hand, but that's obviously terrible.)

@tsee
Copy link
Contributor Author

tsee commented Jan 12, 2017

https://github.com/tsee/C-Blocks/tree/hack_for_symbols

That branch has a hacky workaround.

Somewhere, it seems I broke the symbol table preprocessing, btw.

@tsee
Copy link
Contributor Author

tsee commented Jan 13, 2017

Somewhere, it seems I broke the symbol table preprocessing, btw.

Turns out that I just had an old PerlAPI.xs lying around that was mysteriously not being cleaned up. I think it was an order of rebuilding and git branch switching commands that I used that confused everything. So all is well.

I think until there's a better solution, I'd actually recommend merging the hack_for_symbols branch because it avoids putting the symbols all over the place. I'll submit a pull request.

@run4flat
Copy link
Owner

When names.txt or share/perl.h.cache are not cleaned up, which happened far too frequently for my comfort, I would get terribly obscure link errors. This is in part why I had an explicit removal in the Build.PL file. Actually, it might make sense to reinstate the explicit unlink in the Build.PL file, in addition to the add_to_cleanup. Any objections?

@tsee
Copy link
Contributor Author

tsee commented Jan 13, 2017

No objections at all. Just looked like an old debugging relic to me. :)

@tsee
Copy link
Contributor Author

tsee commented Jan 13, 2017

Quick question: You mentioned you're working on splitting more code out (namely the parser). I had started on that a bit myself. Happy to leave it to you, but I was thinking about prototyping the signature scanning/parsing with the latest proposal I dumped in the other issue thread. It feels like it would be a shame if that ended up reasonable but unmergeable.

What's the status on the split? Should I just go ahead with my experimentation or wait?

(Sorry if that sounds like I'm trying to put pressure on you. Don't mean to. Just don't want to cause more harm than good.)

@run4flat
Copy link
Owner

I haven't actually worked on it yet. The reason I wanted to do it was because it would give me the opportunity to document how the thing works. If you split it out, then maybe more than one person on the planet will see how it works, and think of ways to make it a little less clever :-P

Do let me know if you take it up; I'll also notify here if I start working on it.

@tsee
Copy link
Contributor Author

tsee commented Jan 13, 2017

I can't say that I really understand it in any level of detail. The main issue I had with how it works is that all the different pieces seem very cosy with one another, making it hard to tease them apart.

This seems primarily driven by that everything modifies state in a effectively global c_blocks_data instance and because the parser seems to want to do more than just parse. If more bits of logic were refactored to be pure(er) functions and the code-gen happened strictly after the parsing, I think a lot of this would be simpler to grok (at least for me).

I'll try and move things about a bit and push a branch (provided I have enough time to finish). I'm not convinced that what I'd be pushing would be the best or even a good way to split. But maybe it works as a starting point.

I'll let you know.

@tsee
Copy link
Contributor Author

tsee commented Jan 13, 2017

#24

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

No branches or pull requests

2 participants