Skip to content

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

Closed
wants to merge 1 commit into from
Closed

Conversation

msva
Copy link

@msva msva commented Jun 4, 2015

No description provided.

tp = ngx_timeofday();
#if defined(nginx_version) && (nginx_version >= 1009001)
Copy link
Contributor

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+.

Copy link
Author

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

<...>

@agentzh
Copy link
Contributor

agentzh commented Jun 13, 2015

@msva Yes, like that. But you'd better ensure empty lines around your #if, #else, and #endif for aesthetic reasons.

@msva msva force-pushed the patch-1 branch 2 times, most recently from e80bb14 to 778a1b5 Compare June 13, 2015 09:54
@msva
Copy link
Author

msva commented Jun 13, 2015

here it is ;)

@msva msva force-pushed the patch-1 branch 2 times, most recently from f467b53 to ad6a835 Compare June 13, 2015 09:56
@agentzh
Copy link
Contributor

agentzh commented Jun 13, 2015

@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.

@PiotrSikora ?

@msva
Copy link
Author

msva commented Jun 13, 2015

will it be okay in that way?


#else

ngx_time_t *tp;
Copy link
Contributor

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.

@agentzh
Copy link
Contributor

agentzh commented Jun 13, 2015

@msva No, putting the variable declaration inside the if block also violates the nginx coding style (well, I didn't invent that style).

@msva
Copy link
Author

msva commented Jun 13, 2015

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 ;)

@msva
Copy link
Author

msva commented Jun 13, 2015

Uhm... Did I misunderstood you about what exactly violating style, or do NginX team violates style theyself?
http://hg.nginx.org/nginx/file/6893a1007a7c/src/http/ngx_http_request.c#l204

@agentzh
Copy link
Contributor

agentzh commented Jun 13, 2015

@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

@msva
Copy link
Author

msva commented Jun 13, 2015

So, is it okay now? :)

@agentzh
Copy link
Contributor

agentzh commented Jun 13, 2015

@msva Looking good to me now :)

@msva
Copy link
Author

msva commented Jun 17, 2015

@PiotrSikora ping? :(

@agentzh
Copy link
Contributor

agentzh commented Jul 2, 2015

@msva No worries. I'll look into this.

@agentzh
Copy link
Contributor

agentzh commented Jul 4, 2015

@msva I'm getting the following compilation error on Linux (Fedora 22, gcc 5.1.1) using your branch:

/home/agentzh/git/postgres-nginx-module/src/ngx_postgres_util.c: In function ‘ngx_postgres_upstream_finalize_request’:
/home/agentzh/git/postgres-nginx-module/src/ngx_postgres_util.c:71:33: error: assignment makes integer from pointer without a cast [-Werror=int-conversion]
         u->state->response_time = ngx_timeofday()->msec - u->state->response_time;
                                 ^
cc1: all warnings being treated as errors

Maybe you should use ngx_current_msec instead of ngx_timeofday()->msec here?

agentzh added a commit that referenced this pull request Jul 4, 2015
@agentzh
Copy link
Contributor

agentzh commented Jul 4, 2015

@msva Never mind, I've committed a slightly modified version of your patch to master. Thanks for your contribution!

@agentzh agentzh closed this Jul 4, 2015
@msva msva deleted the patch-1 branch July 4, 2015 09:10
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

Successfully merging this pull request may close these issues.

2 participants