search

ballsteve / xrust

XPath, XQuery, and XSLT for Rust
Apache License 2.0
89 stars 9 forks source link

Cleanup 2023 11 17 #62

Closed ballsteve closed 11 months ago

ballsteve commented 11 months ago

Converted picture parser to use organic parser combinator instead of nom. Moved tree implementations into their own module. Removed obsolete files.

Devasta commented 11 months ago

Hi Steve,

Very sorry about this, the changes look ok but I cannot compile this branch either!

I'll try to get it running and then give proper feedback, but everything I looked at seems fine.

ballsteve commented 11 months ago

I've got a Windows VM, so I'll try and reproduce the problem.

ballsteve commented 11 months ago

I got Rust installed in my Windows VM and started the build. 'cargo build' ran very slowly... it completed overnight (!). Then I started 'cargo test'. This morning, it was still running. 100% CPU, but not using all memory; I thought it might have been taking so long because of paging, but apparently not. I've killed that process, and now I'm going to remove modules until I find the culprit. I suspect the XPath parser, because it has a recursive type and Rust really doesn't like that.

ballsteve commented 11 months ago

I can confirm that the XPath parser is the culprit. Now I just need to figure out what the problem is...

ballsteve commented 11 months ago

I think I've got it fixed now.

In the XPath parser, because the grammar is recursive every function must be Box<dyn ...> instead of impl.

This makes Rust resolve types at runtime, instead of at compile time.

ballsteve commented 11 months ago

Daniel, please retest this branch.

Devasta commented 11 months ago

These changes are looking good. I might adjust the "alt" combinators to all have consistent behavior in a future release, but thats fairly low priority.

Gonna push to dev now.

Devasta commented 11 months ago

Omg, I'm an idiot! Tested cleanup 2023-11-27 locally then merged this one!

I've pushed the right branch now. 😅

ballsteve commented 11 months ago

Sounds like the kind of thing I would do ;-)

Thanks for that. I’m working on a couple of extra features to include in the release, and then we’ll push it through.

Cheers, Steve

On 4 Dec 2023, at 1:33 am, Daniel Murphy @.***> wrote:

Omg, I'm an idiot! Tested cleanup 2023-11-27 locally then merged this one!

I've pushed the right branch now. 😅

— Reply to this email directly, view it on GitHub https://github.com/ballsteve/xrust/pull/62#issuecomment-1837500981, or unsubscribe https://github.com/notifications/unsubscribe-auth/AUCP5QOWE2UOTOGW7D4IXPDYHSEUHAVCNFSM6AAAAAA7PALWB2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZXGUYDAOJYGE. You are receiving this because you were assigned.