-
Notifications
You must be signed in to change notification settings - Fork 126
Fixing compilation with nginx>=1.9.1 #38
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
Conversation
tp = ngx_timeofday(); | ||
#if defined(nginx_version) && (nginx_version >= 1009001) |
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 suggest merge these two #if
into a single one. Also, we don't need the "tp" variable assignment code path for nginx 1.9.1+.
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.
So, if I understand you correct, it would, probably, be something like below, yes?
+#if defined(nginx_version) && (nginx_version >= 1009001)
+ if (u->state && u->state->response_time) {
u->state->response_time = ngx_timeofday()->msec - u->state->response_time; // [!!!] or this line is unneeded entirely?
+#else
if (u->state && u->state->response_sec) {
tp = ngx_timeofday();
u->state->response_sec = tp->sec - u->state->response_sec;
u->state->response_msec = tp->msec - u->state->response_msec;
+#endif
<...>
@msva Yes, like that. But you'd better ensure empty lines around your |
e80bb14
to
778a1b5
Compare
here it is ;) |
f467b53
to
ad6a835
Compare
@msva I think you need to short-circuit the "tv" variable declaration for nginx 1.9.1+ too. It is never used in that code path. Also, again: you'd better ensure empty lines around your #if, #else, and #endif for aesthetic reasons. |
will it be okay in that way? |
|
||
#else | ||
|
||
ngx_time_t *tp; |
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.
Style: no variable declaration in the middle of the function body.
@msva No, putting the variable declaration inside the |
Uhm.. But according to that: http://wiki.nginx.org/CodingStyle (although, there is struct, but not plain variables) it is okay... But ok, I'll think about it ;) |
Uhm... Did I misunderstood you about what exactly violating style, or do NginX team violates style theyself? |
@msva Regarding the empty lines around macro directives, either way is fine. That's why I said it was for aesthetic reasons. For example, http://hg.nginx.org/nginx/file/6893a1007a7c/src/http/ngx_http_request.c#l1833 |
So, is it okay now? :) |
@msva Looking good to me now :) |
@PiotrSikora ping? :( |
@msva No worries. I'll look into this. |
@msva I'm getting the following compilation error on Linux (Fedora 22, gcc 5.1.1) using your branch:
Maybe you should use |
…Misbakh-Soloviov for the original patch in #38.
@msva Never mind, I've committed a slightly modified version of your patch to master. Thanks for your contribution! |
No description provided.