-
Notifications
You must be signed in to change notification settings - Fork 272
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
feature: Add FFI functions for ngx.req.get_post_args #87
base: master
Are you sure you want to change the base?
Conversation
t/request.t
Outdated
location = /t { | ||
set $foo hello; | ||
content_by_lua_block { | ||
local ffi = require "ffi" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't seem to be of any use here, or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, looks like this is a legacy holdover from a number of other tests (I copypasta'd the test structure from the get_args() tests); i'll remove this one and push up a separate PR to get rid of this cruft in the existing tests.
qr/\[TRACE \d+ content_by_lua\(nginx\.conf:\d+\):5 loop\]/ | ||
--- no_error_log | ||
[error] | ||
-- NYI: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a test with get_post_args(n)
and get_post_args(0)
. I definitely think we need those :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test cases in this library for the FFI-version of ngx_lua's Lua API should focus instead of JIT-ability instead of functionality. Functionality tests are already in the ngx_lua module and ngx_lua's test suite can run with lua-resty-core with the following environment setting, for example:
export TEST_NGINX_INIT_BY_LUA="package.path = (package.path or '') .. ';$PWD/../lua-resty-core/lib/?.lua;$PWD/../lua-resty-lrucache/lib/?.lua;' require 'resty.core'"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good!
|
||
local args = new_tab(0, nargs) | ||
for i = 0, nargs - 1 do | ||
local arg = kvbuf[i] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we go against Mike Pall's advice not to cache table lookup in such a hot code path? I am not sure, just to be clear, just raising a concern we previously talked about :)
This happens a bunch in this function, although, I do agree that it makes the code more readable :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think style consistency is important here; this loop is a duplicate of the one for URI args, so this seems the most appropriate choice. I would be interested in looking at some jit dump output of the two approaches and seeing what the functional difference is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm really unsure too, especially since we're dealing with cdata here and not tables...
lib/resty/core/request.lua
Outdated
local FFI_POST_ARGS_OK = 0 | ||
local FFI_POST_ARGS_EMPTY = 1 | ||
local FFI_POST_ARGS_UNREAD = 2 | ||
local FFI_POST_ARGS_IN_FILE = 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No extra error code constants.
@p0pr0ck5 Will you please rebase to the latest master? Sorry for the delay on my side. I'm about to look at it but found it has conflicts with the latest master. Thanks! |
Update function signatures and remove an unused require in tests Remove unwanted constants and refactor error checking
@agentzh rebased and squashed. |
@p0pr0ck5 Cool, thanks! I'll have a look. |
@agentzh any thoughts here? |
No description provided.