Reduce code duplications in _parseRequest

[dirkmueller]

This function had two loops for parsing headers, one used for GET
like requests and one for POST like request. we can combine them
in one, which reduces PROGMEM footprint by over 250 bytes.

[earlephilhower]

Just curious about the headerValue change. Rest seems quite nice!

[devyte]

This is a signature change for a non-private method, and therefore breaking. We can wait to merge for v3, or you could add a compatibility method that forwards from one to the other.

[dirkmueller]

it is source compatible (String(const char*) constructor is implicit). I don't see the need to add a forwarder method - it will just work in any case as far as I can see.

[devyte]

You're thinking of method call, I'm referring to inheritance. This change can cause a derived class to no longer compile. Trivial example with std::string instead of String, but the principle is the same:

struct Base
{
    void method(const char * arg) {std::cout << "Base arg=" << arg << std::endl;}
};

struct Derived : Base
{
//    void method(const char *arg) 
    void method(const std::string &arg) 
    {
        std::cout << "strlen=" << strlen(arg) << std::endl;
        Base::method(arg);
    }
};

[devyte]

Did you test that moving this condition still results in correct function as before for all methods?

[dirkmueller]

I did not test all methods. I did test GET and POST though in the scope of my application and it worked fine.

Another way to put it is however that this "if condition" didn't actually move. what happened is that the header parsing part that was used in the POST/PUT/PATCH/DELETE code was moved up front, and then the rest of the code remained the same. this allowed to remove the header parsing that was done for GET/HEAD case as it was duplicate now.

Personally I think thats a relatively low risk change because the two header parsing loops were doing mostly the same, except that POST/PUT/PATCH/DELETE was checking for additional headers and parsed them. It now does so in all cases, but since it is extremely unlikely to get the additional header fields send by GET/HEAD requests, it will not matter in practice. and even if we get them, the parsed result is ignored as the if() condition remained the same and the parsed resulting values are not further evaluated.

[devyte]

@dirkmueller we're now merging for v3, so it's ok to break things a bit. What's the status here? Did you get to testing all the methods?

[dirkmueller]

yes, I tested this back then successfully. I'll have to rebase and let you know how retesting goes, currently working on another project.