Skip to content
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

Add StatusCode enum #1739

Merged
merged 4 commits into from
Dec 19, 2023
Merged

Add StatusCode enum #1739

merged 4 commits into from
Dec 19, 2023

Conversation

bugdea1er
Copy link
Contributor

@bugdea1er bugdea1er commented Dec 18, 2023

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 Outdated Show resolved Hide resolved
httplib.h Outdated Show resolved Hide resolved
@bugdea1er
Copy link
Contributor Author

bugdea1er commented Dec 18, 2023

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
Copy link
Owner

yhirose commented Dec 19, 2023

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
Copy link
Contributor Author

bugdea1er commented Dec 19, 2023

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
Copy link
Owner

yhirose commented Dec 19, 2023

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/[email protected]/include -L/usr/local/opt/[email protected]/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
Copy link
Owner

yhirose commented Dec 19, 2023

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/[email protected]/include -L/usr/local/opt/[email protected]/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
Copy link
Contributor Author

bugdea1er commented Dec 19, 2023

@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
Copy link
Owner

yhirose commented Dec 19, 2023

@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
Copy link
Contributor Author

@yhirose I added number suffixes to enum constants

@yhirose
Copy link
Owner

yhirose commented Dec 19, 2023

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

@bugdea1er
Copy link
Contributor Author

@yhirose ok, I removed them

@yhirose
Copy link
Owner

yhirose commented Dec 19, 2023

@bugdea1er thanks for your quick modification!

@yhirose yhirose merged commit d39fda0 into yhirose:master Dec 19, 2023
4 checks passed
@bugdea1er
Copy link
Contributor Author

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

@bugdea1er bugdea1er deleted the status-code branch December 19, 2023 23:12
@yhirose
Copy link
Owner

yhirose commented Dec 19, 2023

@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
Copy link
Contributor Author

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

The second one will appear in a few days

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