'ssize_t' should be used for h2o_find_header/h2o_find_header_by_str #2033

chenbd · 2019-04-14T14:08:49Z

This PR trying clean up some code for 'lib/core/headers.c'

chenbd · 2019-04-15T01:54:46Z

Not sure about the ci error, Issue in test code??

   #   Failed test 'upper bound'
        #   at t/50server-timing.t line 153.
        #     '1304.384'
        #         <
        #     '1100'
        # Looks like you failed 1 test of 3.
    
    #   Failed test 'response'
    #   at t/50server-timing.t line 154.
        
        #   Failed test 'upper bound'
        #   at t/50server-timing.t line 153.
        #     '2303.802'
        #         <
        #     '2100'
        # Looks like you failed 1 test of 3.
    
    #   Failed test 'total'
    #   at t/50server-timing.t line 154.
    # Looks like you failed 2 tests of 9

kazuho

Thank you for spotting the issue and working on the fix.

I've left my comments on API changes that I might be concerned about, but things look OK otherwise.

kazuho · 2019-04-15T02:02:01Z

include/h2o/header.h

@@ -106,7 +106,7 @@ ssize_t h2o_set_header_token(h2o_mem_pool_t *pool, h2o_headers_t *headers, const
 /**
 * deletes a header from list
 */
-ssize_t h2o_delete_header(h2o_headers_t *headers, ssize_t cursor);
+void h2o_delete_header(h2o_headers_t *headers, ssize_t cursor);


Is there a necessity to change the API? Unless there is, I'd prefer keeping the API as-is (consider the users of libh2o).

Yes, fixed.
Do we need consider keeping API stable from now on?
And for this function the return value seems useless because i can not image how the library user can use the return value(perhaps call h2o_delete_header again with return value?).
Anyway, leave it as-is.

kazuho · 2019-04-15T02:06:08Z

lib/core/headers.c

@@ -24,7 +24,7 @@
 #include "h2o.h"

 static ssize_t add_header(h2o_mem_pool_t *pool, h2o_headers_t *headers, h2o_iovec_t *name, const char *orig_name, const char *value,
-                          size_t value_len, h2o_header_flags_t flags)


I think I would prefer retaining the argument. It's true that we are not using it now, but it's harmless, it's good to have a constructor function that accepts all the field values as arguments. Performance-wise, it does not matter.

kazuho · 2019-04-15T06:31:24Z

Do we need consider keeping API stable from now on?

Changing the API is fine if there is a necessity to do so. We prefer moving forward than getting ossified. However, I think we should avoid making excessive changes, because there could be users depending on the current form.

kazuho · 2019-04-19T00:19:25Z

Thank you for all your work on the PR. Merged.

chenbd added 2 commits April 14, 2019 21:33

'ssize_t' should be used for h2o_find_header/h2o_find_header_by_str

5b3dff6

more clean up for 'lib/core/headers.c'

8d476d2

kazuho reviewed Apr 15, 2019

View reviewed changes

fix PR issue: keep API as-is

fadf704

kazuho merged commit 8cf3f6c into h2o:master Apr 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

'ssize_t' should be used for h2o_find_header/h2o_find_header_by_str #2033

'ssize_t' should be used for h2o_find_header/h2o_find_header_by_str #2033

'ssize_t' should be used for h2o_find_header/h2o_find_header_by_str #2033

'ssize_t' should be used for h2o_find_header/h2o_find_header_by_str #2033

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment