Add StatusCode enum #1739

bugdea1er · 2023-12-18T22:50:38Z

Hi there!

C++ Core Guidelines recommend avoiding "magic constants" (link) and clang-tidy enforces this. I added a enum with Response Status Codes to use them instead of plain numbers in dependent code, please let me know if you would like to use them in httplib too.

~~Also I changed some of status messages based on RFC 9110~~
Moved RFC 9110 changes to a separate pr #1740

httplib.h

bugdea1er · 2023-12-18T23:00:19Z

I would suggest to make this enum class and use this new type for Response::status field, but I don't know if it would be a breaking change

yhirose · 2023-12-19T04:09:39Z

Also I changed some of status messages based on RFC 9110

Could you move the change to a separate pull request? I'll merge it right away!

I would suggest to make this enum class and use this new type for Response::status field, but I don't know if it would be a breaking change

It's a absolutely big breaking change. I understand it's a good practice to enum values instead of hardcoded numbers though, I'll keep the current code as it is since I would not like to break the existing users' code at all. Thanks for your understanding.

bugdea1er · 2023-12-19T07:43:19Z

Could you move the change to a separate pull request? I'll merge it right away!

#1740

It's a absolutely big breaking change. I understand it's a good practice to enum values instead of hardcoded numbers though, I'll keep the current code as it is since I would not like to break the existing users' code at all. Thanks for your understanding.

So a plane enum (like in this pr) is okay with you? All the types stays int and users can use this constats without breaking their code?

Btw, I checked with our existing code, compiles fine before and after using this enum in Request::status assignments. C++ allows implicit enum to int conversion.

yhirose · 2023-12-19T15:25:37Z

Btw, I checked with our existing code, compiles fine before and after using this enum in Request::status assignments. C++ allows implicit enum to int conversion.

Here are compile errors which shows the breaking change when compiling 'test/test.cc'.

  ...
  LoopDetected = 508,                  ///< 508 Loop Detected
  NotExtended = 510,                   ///< 510 Not Extended
  NetworkAuthenticationRequired = 511, ///< 511 Network Authentication Required
};

struct Response {
  std::string version;
  // int status = -1;
  StatusCode status = -1;
  std::string reason;
  Headers headers;
  ...

~/Projects/cpp-httplib/test$ make
clang++ -o test -I.. -g -std=c++11 -I. -Wall -Wextra -Wtype-limits -Wconversion -Wshadow  test.cc include_httplib.cc gtest/gtest-all.cc gtest/gtest_main.cc -DCPPHTTPLIB_OPENSSL_SUPPORT -I/usr/local/opt/openssl@1.1/include -L/usr/local/opt/openssl@1.1/lib -lssl -lcrypto -DCPPHTTPLIB_USE_CERTS_FROM_MACOSX_KEYCHAIN -framework CoreFoundation -framework Security -DCPPHTTPLIB_ZLIB_SUPPORT -lz -DCPPHTTPLIB_BROTLI_SUPPORT -I/usr/local/opt/brotli/include -L/usr/local/opt/brotli/lib -lbrotlicommon -lbrotlienc -lbrotlidec -pthread
In file included from test.cc:1:
../httplib.h:590:23: error: cannot initialize a member subobject of type 'StatusCode' with an rvalue of type 'int'
  StatusCode status = -1;
                      ^~
../httplib.h:4247:9: error: no viable overloaded '='
    res = new_res;
    ~~~ ^ ~~~~~~~
../httplib.h:7087:20: note: in instantiation of function template specialization 'httplib::detail::redirect<httplib::ClientImpl>' requested here
    return detail::redirect(*this, req, res, path, location, error);
                   ^
../httplib.h:622:13: note: candidate function not viable: expects an rvalue for 1st argument
  Response &operator=(Response &&) = default;
            ^
../httplib.h:4247:9: error: no viable overloaded '='
    res = new_res;
    ~~~ ^ ~~~~~~~
../httplib.h:7094:22: note: in instantiation of function template specialization 'httplib::detail::redirect<httplib::SSLClient>' requested here
      return detail::redirect(cli, req, res, path, location, error);
                     ^
../httplib.h:622:13: note: candidate function not viable: expects an rvalue for 1st argument
  Response &operator=(Response &&) = default;
            ^
3 errors generated.
In file included from include_httplib.cc:5:
../httplib.h:590:23: error: cannot initialize a member subobject of type 'StatusCode' with an rvalue of type 'int'
  StatusCode status = -1;
                      ^~
../httplib.h:4247:9: error: no viable overloaded '='
    res = new_res;
    ~~~ ^ ~~~~~~~
...

yhirose · 2023-12-19T15:28:45Z

Even if I added NotSet = -1, we get lots of compile errors in 'test/test.cc'.

  ...
  NetworkAuthenticationRequired = 511, ///< 511 Network Authentication Required

  NotSet = -1
};

struct Response {
  std::string version;
  // int status = -1;
  StatusCode status = StatusCode::NotSet;
  std::string reason;
  ...

~/Projects/cpp-httplib/test$ make
clang++ -o test -I.. -g -std=c++11 -I. -Wall -Wextra -Wtype-limits -Wconversion -Wshadow  test.cc include_httplib.cc gtest/gtest-all.cc gtest/gtest_main.cc -DCPPHTTPLIB_OPENSSL_SUPPORT -I/usr/local/opt/openssl@1.1/include -L/usr/local/opt/openssl@1.1/lib -lssl -lcrypto -DCPPHTTPLIB_USE_CERTS_FROM_MACOSX_KEYCHAIN -framework CoreFoundation -framework Security -DCPPHTTPLIB_ZLIB_SUPPORT -lz -DCPPHTTPLIB_BROTLI_SUPPORT -I/usr/local/opt/brotli/include -L/usr/local/opt/brotli/lib -lbrotlicommon -lbrotlienc -lbrotlidec -pthread
In file included from test.cc:1:
../httplib.h:5315:22: error: assigning to 'StatusCode' from incompatible type 'int'
      this->status = stat;
                     ^~~~
../httplib.h:5317:22: error: assigning to 'StatusCode' from incompatible type 'int'
      this->status = 302;
                     ^~~
../httplib.h:6096:22: error: assigning to 'StatusCode' from incompatible type 'int'
        res.status = 413; // NOTE: should be 414?
                     ^~~
../httplib.h:6127:20: error: assigning to 'StatusCode' from incompatible type 'int'
      res.status = 400;
                   ^~~
../httplib.h:6156:8: error: no matching function for call to 'read_content'
  if (!detail::read_content(strm, req, payload_max_length_, res.status, nullptr,
       ^~~~~~~~~~~~~~~~~~~~
../httplib.h:3982:6: note: candidate function template not viable: no known conversion from 'StatusCode' to 'int &' for 4th argument
bool read_content(Stream &strm, T &x, size_t payload_max_length, int &status,
     ^
../httplib.h:6163:20: error: assigning to 'StatusCode' from incompatible type 'int'
      res.status = 400;
                   ^~~
../httplib.h:6405:16: error: assigning to 'StatusCode' from incompatible type 'int'
  res.status = 400;
               ^~~
../httplib.h:6488:22: error: assigning to 'StatusCode' from incompatible type 'int'
        res.status = 416;
                     ^~~
...

bugdea1er · 2023-12-19T18:45:34Z

@yhirose I meant just an assignment of this enum's values to int Response::status, int on the left and enum on the right

I'm not suggesting replacing the Response::status type to enum StatusCode. I'm just suggesting an enum with constants like StatusCode::Forbidden, which the user can then use with the Response struct unchanged

I don't get any errors here:

struct Response {
  std::string version;
  int status = -1;
  std::string reason;
  ...

response.status = httplib::StatusCode::Forbidden

Does this work for you? I believe this should work on every C++ compiler

yhirose · 2023-12-19T19:53:47Z

@bugdea1er thanks for the clarification. I know understand what you meant. :)

Still I am not very comfortable with the enum though. It's because the enum is lacking the category information. The first digit of each status code gives me very valuable information. As soon as I see '2', I can quickly recognize the message is a 'Success' code, and '4' makes me understand it's a request problem. '5' gives me information that something bad happening in the server, and '3' is a redirect code.

So if the enum looks like the following, it might be acceptable...

enum StatusCode {
  ...
  NotFound_404 = 404,
  ...
};

int status = StatusCode::NotFound_404;

bugdea1er · 2023-12-19T21:59:24Z

@yhirose I added number suffixes to enum constants

yhirose · 2023-12-19T22:10:02Z

@bugdea1er also we don't need the comments like ///< 100 Continue.

bugdea1er · 2023-12-19T22:13:15Z

@yhirose ok, I removed them

yhirose · 2023-12-19T22:57:27Z

@bugdea1er thanks for your quick modification!

bugdea1er · 2023-12-19T23:12:17Z

@yhirose I can create a follow-up pr where I replace every number status code with an enum constant in cpp-httplib code

Should I?

yhirose · 2023-12-19T23:23:00Z

@bugdea1er could you make two separate pull requests, the first one is for 'httplib.h' and the other one is for files in 'test' and 'example' directories? Thanks!

bugdea1er · 2023-12-20T00:21:50Z

@yhirose Sure thing! The first one for httplib.h is already here: #1742

The second one will appear in a few days

bugdea1er commented Dec 18, 2023

View reviewed changes

httplib.h Outdated Show resolved Hide resolved

httplib.h Outdated Show resolved Hide resolved

Add StatusCode enum

833fe8e

Remove changes on RFC 9110

9bdc0f5

Add number suffixes to StatusCode constants

382dc48

Remove docs for StatusCode constants

b1def99

yhirose merged commit d39fda0 into yhirose:master Dec 19, 2023
4 checks passed

bugdea1er deleted the status-code branch December 19, 2023 23:12

bugdea1er mentioned this pull request Dec 20, 2023

Use StatusCode in tests and examples #1743

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add StatusCode enum #1739

Add StatusCode enum #1739

bugdea1er commented Dec 18, 2023 •

edited

bugdea1er commented Dec 18, 2023 •

edited

yhirose commented Dec 19, 2023 •

edited

bugdea1er commented Dec 19, 2023 •

edited

yhirose commented Dec 19, 2023

yhirose commented Dec 19, 2023 •

edited

bugdea1er commented Dec 19, 2023 •

edited

yhirose commented Dec 19, 2023

bugdea1er commented Dec 19, 2023

yhirose commented Dec 19, 2023

bugdea1er commented Dec 19, 2023

yhirose commented Dec 19, 2023

bugdea1er commented Dec 19, 2023

yhirose commented Dec 19, 2023

bugdea1er commented Dec 20, 2023

Add StatusCode enum #1739

Add StatusCode enum #1739

Conversation

bugdea1er commented Dec 18, 2023 • edited

bugdea1er commented Dec 18, 2023 • edited

yhirose commented Dec 19, 2023 • edited

bugdea1er commented Dec 19, 2023 • edited

yhirose commented Dec 19, 2023

yhirose commented Dec 19, 2023 • edited

bugdea1er commented Dec 19, 2023 • edited

yhirose commented Dec 19, 2023

bugdea1er commented Dec 19, 2023

yhirose commented Dec 19, 2023

bugdea1er commented Dec 19, 2023

yhirose commented Dec 19, 2023

bugdea1er commented Dec 19, 2023

yhirose commented Dec 19, 2023

bugdea1er commented Dec 20, 2023

bugdea1er commented Dec 18, 2023 •

edited

bugdea1er commented Dec 18, 2023 •

edited

yhirose commented Dec 19, 2023 •

edited

bugdea1er commented Dec 19, 2023 •

edited

yhirose commented Dec 19, 2023 •

edited

bugdea1er commented Dec 19, 2023 •

edited