-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: Optionally pass regex capture groups to router handler #18
Conversation
This is due to a gotcha with std::regex_match. The contents of the output match_results are undefined if the input string is destroyed :facepalm:
Also includes test for request which reproduces the uri bug
Turns out both branches of std::conditional must be valid statements
Don't merge this yet, investigating an issue with subrouters, not sure if this caused it or its an error in my server code |
OK that should do it. I also fixed #10 since it kept coming up when debugging this. Hope thats ok, if not I'll cherry-pick it and make a new pull. |
I currently can't test this on my end before merging as I am now longer to build with MinGW since merging #14:
This is with the corresponding |
I've tried to compile with Not sure how to handle this right now :/ |
Would making it a standalone (dll) library help? So Could also try breaking up connection_detector.cpp. I suspect that one is due to |
Okay so I can't repro this. https://github.com/0x00002a/malloy/tree/fix-mingw-compile has the fix I suggested above (putting handler in its own file) but #14 also compiles for me (with tls on and malloy-example-server-session). It takes about 10 minutes and uses most of my 16gb of ram, but it does compile. It may be because I'm using ninja or might be because I've got a shorter path but I'm just guessing here. My full env:
Also the CI mingw is dead because the choco version of mingw doens't include thread (at least not with gcc 10). Not sure how to use msys with github actions but I'll look into it at some point. Also also the example doesn't compile by default, I have to add a |
As far as I know this wont help because the error is caused by limitations of a TU, not the finalized binary. I tried it anyway but it didn't work as expected.
I am actually glad to hear this :p
Would you like to PR that? So I picked your branch and tried to build: Still failing with the same problem. |
So I removed the
This is using your |
I created a separate issue for this to keep things easier to maintain: #19 |
Is this ready to be merged from your side? |
Yep should be good. I've been using this branch for a while now on msvc |
Fixed in 92bfd84. |
This allows handlers for
router::add
to have an optional second parameter. To support this,endpoint_http_regex
is now templated on whether the handler uses the extra argument. It uses this to decide at compile-time whether to do the extra regex match onhandle
. Note it uses no caching and will (redundently) match the same string twice. I considered adding caching but I think that borders on premature optimsation, especially since it would mean removing theconst
ness ofhandle(...)
etc al.I have added tests for it which currently pass. It no longer uses
uri
when matching due to a bug I found inrequest
when writing the tests and becauseI'm not sure what the point is (validation?).ok so that was needed for subrouters. I've reset it to using uri and the tests are using thebeast::...::request
ctor to get around the bug. The bug is reproduced in a test I've included in this patch (which currently fails). The problem is thatrequest
's ctor that takestarget
does not inititalisem_uri
.Also it requires /bigobj to compile on MSVC. I'm not sure if thats due to this or #14. Either way,
malloy-objs
should make that a public option since anything including its headers will need it.closes: #12, closes: #10