ronantakizawa/codereview-bench huggingface dataset:
Repository: oatpp/oatpp
File: src/oatpp/core/data/mapping/type/Any.cpp (C++)

Code before:
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 *
 ***************************************************************************/

#include "Any.hpp"

namespace oatpp { namespace data { namespace mapping { namespace type {

namespace __class {
  const ClassId Any::CLASS_ID("Any");
}

Any::Any()
  : ObjectWrapper(__class::Any::getType())
{}

Any::Any(std::nullptr_t) : Any() {}

Any::Any(const std::shared_ptr<AnyHandle>& handle, const Type* const type)
  : ObjectWrapper(handle, __class::Any::getType())
{
  //TODO: is there indeed no use for |type| parameter passed above?
  (void)type;
}

Any::Any(const Any& other)
  : ObjectWrapper(std::make_shared<AnyHandle>(other.m_ptr->ptr, other.m_ptr->type), __class::Any::getType())
{}

Any::Any(Any&& other)
  : ObjectWrapper(std::move(other.m_ptr),  __class::Any::getType())
{}

const Type* Any::getStoredType() const {
  if(m_ptr) {
    return m_ptr->type;
  }
  return nullptr;
}

Any& Any::operator=(std::nullptr_t) {
  m_ptr.reset();
  return *this;
}

Any& Any::operator=(const Any& other) {
  if(other) {

Review comment (suggestion):
As an `ObjectWrapper`, `Any` must have this constructor. It is used in `ObjectWrapper.staticCast<T>(...)` method.
However, unlike other `ObjectWrapper`s `Any` won't change its `valueType`.
`Any` is always object wrapper above `AnyHandler`.


Code after:
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 *
 ***************************************************************************/

#include "Any.hpp"

namespace oatpp { namespace data { namespace mapping { namespace type {

namespace __class {
  const ClassId Any::CLASS_ID("Any");
}

Any::Any()
  : ObjectWrapper(__class::Any::getType())
{}

Any::Any(std::nullptr_t) : Any() {}

Any::Any(const std::shared_ptr<AnyHandle>& handle, const Type* const type)
  : ObjectWrapper(handle, __class::Any::getType())
{
  // As an ObjectWrapper, Any must have this constructor. It is used in ObjectWrapper.staticCast<T>(...) method.
  // However, unlike other ObjectWrappers Any won't change its valueType.
  // Any is always object wrapper above AnyHandler.
  (void)type;
}

Any::Any(const Any& other)
  : ObjectWrapper(std::make_shared<AnyHandle>(other.m_ptr->ptr, other.m_ptr->type), __class::Any::getType())
{}

Any::Any(Any&& other)
  : ObjectWrapper(std::move(other.m_ptr),  __class::Any::getType())
{}

const Type* Any::getStoredType() const {
  if(m_ptr) {
    return m_ptr->type;
  }
  return nullptr;
}

Any& Any::operator=(std::nullptr_t) {
  m_ptr.reset();
  return *this;
}
Repository: oatpp/oatpp
File: src/oatpp/network/client/SimpleTCPConnectionProvider.cpp (C++)

Code before:

std::shared_ptr<oatpp::data::stream::IOStream> SimpleTCPConnectionProvider::getConnection(){

  auto portStr = oatpp::utils::conversion::int32ToStr(m_port);

  struct addrinfo hints;

  memset(&hints, 0, sizeof(struct addrinfo));
  hints.ai_family = AF_UNSPEC;
  hints.ai_socktype = SOCK_STREAM;
  hints.ai_flags = 0;
  hints.ai_protocol = 0;

  struct addrinfo* result;
  auto res = getaddrinfo(m_host->c_str(), portStr->c_str(), &hints, &result);

  if (res != 0) {
    throw std::runtime_error("[oatpp::network::client::SimpleTCPConnectionProvider::getConnection()]. Error. Call to getaddrinfo() faild.");
  }

  if (result == nullptr) {
    throw std::runtime_error("[oatpp::network::client::SimpleTCPConnectionProvider::getConnection()]. Error. Call to getaddrinfo() returned no results.");
  }

  struct addrinfo* currResult = result;
  oatpp::v_io_handle clientHandle = oatpp::v_io_handle(-1);

  while(currResult != nullptr) {

    clientHandle = socket(currResult->ai_family, currResult->ai_socktype, currResult->ai_protocol);

    if(clientHandle >= 0) {

      if(connect(clientHandle, currResult->ai_addr, (int)currResult->ai_addrlen) == 0) {
        break;
      } else {
#if defined(WIN32) || defined(_WIN32)
		    ::closesocket(clientHandle);
#else
        ::close(clientHandle);
#endif
      }

    }

    currResult = currResult->ai_next;

  }

  freeaddrinfo(result);


Review comment (suggestion):
Let's use a new `INVALID_IO_HANDLE` constant here, from the previous comment.

Code after:

std::shared_ptr<oatpp::data::stream::IOStream> SimpleTCPConnectionProvider::getConnection(){

  auto portStr = oatpp::utils::conversion::int32ToStr(m_port);

  struct addrinfo hints;

  memset(&hints, 0, sizeof(struct addrinfo));
  hints.ai_family = AF_UNSPEC;
  hints.ai_socktype = SOCK_STREAM;
  hints.ai_flags = 0;
  hints.ai_protocol = 0;

  struct addrinfo* result;
  auto res = getaddrinfo(m_host->c_str(), portStr->c_str(), &hints, &result);

  if (res != 0) {
    throw std::runtime_error("[oatpp::network::client::SimpleTCPConnectionProvider::getConnection()]. Error. Call to getaddrinfo() faild.");
  }

  if (result == nullptr) {
    throw std::runtime_error("[oatpp::network::client::SimpleTCPConnectionProvider::getConnection()]. Error. Call to getaddrinfo() returned no results.");
  }

  struct addrinfo* currResult = result;
  oatpp::v_io_handle clientHandle = INVALID_IO_HANDLE;

  while(currResult != nullptr) {

    clientHandle = socket(currResult->ai_family, currResult->ai_socktype, currResult->ai_protocol);

    if(clientHandle >= 0) {

      if(connect(clientHandle, currResult->ai_addr, (int)currResult->ai_addrlen) == 0) {
        break;
      } else {
#if defined(WIN32) || defined(_WIN32)
		    ::closesocket(clientHandle);
#else
        ::close(clientHandle);
#endif
      }

    }

    currResult = currResult->ai_next;

  }

  freeaddrinfo(result);
Repository: oatpp/oatpp
File: src/oatpp/network/server/Server.cpp (C++)

Code before:

void Server::mainLoop(Server *instance){
  
  instance->setStatus(STATUS_STARTING, STATUS_RUNNING);

  std::shared_ptr<const std::unordered_map<oatpp::String, oatpp::String>> params;

  while(instance->getStatus() == STATUS_RUNNING) {
    
    auto connection = instance->m_connectionProvider->getConnection();

    if (connection) {
      if(instance->getStatus() == STATUS_RUNNING){
        instance->m_connectionHandler->handleConnection(connection, params /* null params */);
      } else {
        OATPP_LOGD("Server", "Already stopped. Closing connection...");
      }
    }
    
  }

  instance->setStatus(STATUS_DONE);

}

void Server::run(bool blocking) {
  if (blocking) {
    m_blocking = blocking;
    setStatus(STATUS_CREATED, STATUS_STARTING);
    mainLoop(this);
  } else {
    switch (getStatus()) {
      case STATUS_STARTING:
      case STATUS_RUNNING:
        // Maybe throw an exception here instead of just silently return?
        return;
    }
    m_blocking = blocking;
    setStatus(STATUS_CREATED, STATUS_STARTING);
    m_thread = std::thread(mainLoop, this);
  }
}
  
void Server::stop(){
  setStatus(STATUS_STOPPING);
  if (!m_blocking && m_thread.joinable()) {
      m_thread.join();
  }
}

bool Server::setStatus(v_int32 expectedStatus, v_int32 newStatus){

Review comment (suggestion):
The name "blocking"/"non-blocking" is confusing to me in this context.
I think it's better to describe exactly what it's going to do:

```cpp
Server::run(bool createNewThread);
```


Code after:
void Server::mainLoop(Server *instance) {

  instance->setStatus(STATUS_STARTING, STATUS_RUNNING);

  std::shared_ptr<const std::unordered_map<oatpp::String, oatpp::String>> params;

  while (instance->getStatus() == STATUS_RUNNING) {

    auto connection = instance->m_connectionProvider->getConnection();

    if (connection) {
      if (instance->getStatus() == STATUS_RUNNING) {
        instance->m_connectionHandler->handleConnection(connection, params /* null params */);
      } else {
        OATPP_LOGD("Server", "Already stopped. Closing connection...");
      }
    }

  }

  instance->setStatus(STATUS_DONE);

}

void Server::run(bool startAsNewThread) {
  std::unique_lock<std::mutex> ul(m_mutex);
  switch (getStatus()) {
    case STATUS_STARTING:
      throw std::runtime_error("[oatpp::network::server::run()] Error. Server already starting");
    case STATUS_RUNNING:
      throw std::runtime_error("[oatpp::network::server::run()] Error. Server already started");
  }
  if (startAsNewThread) {
    m_threaded = startAsNewThread;
    setStatus(STATUS_CREATED, STATUS_STARTING);
    m_thread = std::thread(mainLoop, this);
  } else {
    m_threaded = startAsNewThread;
    setStatus(STATUS_CREATED, STATUS_STARTING);
    ul.unlock(); // early unlock
    mainLoop(this);
  }
}

void Server::stop() {
  std::lock_guard<std::mutex> lg(m_mutex);
  switch (getStatus()) {
    case STATUS_CREATED:
      return;
    case STATUS_STARTING:
    case STATUS_RUNNING:
Repository: oatpp/oatpp
File: src/oatpp/network/server/Server.cpp (C++)

Code before:
  }

  instance->setStatus(STATUS_DONE);

}

void Server::run(bool blocking) {
  if (blocking) {
    m_blocking = blocking;
    setStatus(STATUS_CREATED, STATUS_STARTING);
    mainLoop(this);
  } else {
    switch (getStatus()) {
      case STATUS_STARTING:
      case STATUS_RUNNING:
        // Maybe throw an exception here instead of just silently return?
        return;
    }
    m_blocking = blocking;
    setStatus(STATUS_CREATED, STATUS_STARTING);
    m_thread = std::thread(mainLoop, this);
  }
}
  
void Server::stop(){
  setStatus(STATUS_STOPPING);
  if (!m_blocking && m_thread.joinable()) {
      m_thread.join();
  }
}

bool Server::setStatus(v_int32 expectedStatus, v_int32 newStatus){
  v_int32 expected = expectedStatus;
  return m_status.compare_exchange_weak(expected, newStatus);
}
  
void Server::setStatus(v_int32 status){
  m_status.store(status);
}
  
v_int32 Server::getStatus(){
  return m_status.load();
}

Server::~Server() {
  switch (getStatus()) {
    case STATUS_STARTING:
    case STATUS_RUNNING:
      stop();
      break;
    default:

Review comment (suggestion):
What if we have stop method like this:

```cpp
void Server::stop(bool wait = true);
```

if wait == true then wait for the server to stop.

Code after:

  instance->setStatus(STATUS_DONE);

}

void Server::run(bool startAsNewThread) {
  std::unique_lock<std::mutex> ul(m_mutex);
  switch (getStatus()) {
    case STATUS_STARTING:
      throw std::runtime_error("[oatpp::network::server::run()] Error. Server already starting");
    case STATUS_RUNNING:
      throw std::runtime_error("[oatpp::network::server::run()] Error. Server already started");
  }
  if (startAsNewThread) {
    m_threaded = startAsNewThread;
    setStatus(STATUS_CREATED, STATUS_STARTING);
    m_thread = std::thread(mainLoop, this);
  } else {
    m_threaded = startAsNewThread;
    setStatus(STATUS_CREATED, STATUS_STARTING);
    ul.unlock(); // early unlock
    mainLoop(this);
  }
}

void Server::stop() {
  std::lock_guard<std::mutex> lg(m_mutex);
  switch (getStatus()) {
    case STATUS_CREATED:
      return;
    case STATUS_STARTING:
    case STATUS_RUNNING:
      setStatus(STATUS_STOPPING);
      break;
  }

  if (m_threaded && m_thread.joinable()) {
    m_thread.join();
  }
}

bool Server::setStatus(v_int32 expectedStatus, v_int32 newStatus) {
  v_int32 expected = expectedStatus;
  return m_status.compare_exchange_weak(expected, newStatus);
}

void Server::setStatus(v_int32 status) {
  m_status.store(status);
}

v_int32 Server::getStatus() {
Repository: oatpp/oatpp
File: src/oatpp/network/server/Server.cpp (C++)

Code before:
      m_thread.join();
  }
}

bool Server::setStatus(v_int32 expectedStatus, v_int32 newStatus){
  v_int32 expected = expectedStatus;
  return m_status.compare_exchange_weak(expected, newStatus);
}
  
void Server::setStatus(v_int32 status){
  m_status.store(status);
}
  
v_int32 Server::getStatus(){
  return m_status.load();
}

Server::~Server() {
  switch (getStatus()) {
    case STATUS_STARTING:
    case STATUS_RUNNING:
      stop();
      break;
    default:
      // Just check if the thread is still alive and wait for it to exit
      if (!m_blocking && m_thread.joinable()) {
        m_thread.join();
      }
  }
}

}}}


Review comment (suggestion):
Let's keep all `stop()` relates checks in the stop method.
Let's have the destructor to be as follows:

```cpp
Server::~Server() {
  stop(); // wait here if server was not stopped
}
```

Code after:
  switch (getStatus()) {
    case STATUS_CREATED:
      return;
    case STATUS_STARTING:
    case STATUS_RUNNING:
      setStatus(STATUS_STOPPING);
      break;
  }

  if (m_threaded && m_thread.joinable()) {
    m_thread.join();
  }
}

bool Server::setStatus(v_int32 expectedStatus, v_int32 newStatus) {
  v_int32 expected = expectedStatus;
  return m_status.compare_exchange_weak(expected, newStatus);
}

void Server::setStatus(v_int32 status) {
  m_status.store(status);
}

v_int32 Server::getStatus() {
  return m_status.load();
}

Server::~Server() {
  stop();
}

}}}
Repository: oatpp/oatpp
File: src/oatpp/web/server/HttpProcessor.cpp (C++)

Code before:

  response->send(resources.connection.get(), &resources.headersOutBuffer, contentEncoderProvider.get());

  return connectionState;

}

////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
// Task

HttpProcessor::Task::Task(const std::shared_ptr<Components>& components,
                          const std::shared_ptr<oatpp::data::stream::IOStream>& connection,
                          const std::shared_ptr<std::atomic_ulong>& taskCounter)
  : m_components(components)
  , m_connection(connection)
  , m_counter(taskCounter)
{}

void HttpProcessor::Task::run(){

  (*m_counter)++;

  m_connection->initContexts();

  ProcessingResources resources(m_components, m_connection);

  ConnectionState connectionState;

  try {

    do {

      connectionState = HttpProcessor::processNextRequest(resources);

    } while (connectionState == ConnectionState::ALIVE);

  } catch (...) {
    // DO NOTHING
  }

  (*m_counter)--;

}

////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
// HttpProcessor::Coroutine

HttpProcessor::Coroutine::Coroutine(const std::shared_ptr<Components>& components,
                                    const std::shared_ptr<oatpp::data::stream::IOStream>& connection,
                                    const std::shared_ptr<std::atomic_ulong>& taskCounter)
  : m_components(components)

Review comment (suggestion):
Should be incremented in the constructor of the `Task` or else,
someone can successfully call `stop()` before the thread started and `run()` was called.

Code after:

  response->send(resources.connection.get(), &resources.headersOutBuffer, contentEncoderProvider.get());

  return connectionState;

}

////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
// Task

HttpProcessor::Task::Task(const std::shared_ptr<Components>& components,
                          const std::shared_ptr<oatpp::data::stream::IOStream>& connection,
                          std::atomic_long *taskCounter)
  : m_components(components)
  , m_connection(connection)
  , m_counter(taskCounter)
{
  (*m_counter)++;
}

HttpProcessor::Task::Task(const HttpProcessor::Task &copy)
  : m_components(copy.m_components)
  , m_connection(copy.m_connection)
  , m_counter(copy.m_counter)
{
  (*m_counter)++;
}

HttpProcessor::Task::Task(HttpProcessor::Task &&move)
  : m_components(std::move(move.m_components))
  , m_connection(std::move(move.m_connection))
  , m_counter(move.m_counter)
{
  move.m_counter = nullptr;
}

HttpProcessor::Task &HttpProcessor::Task::operator=(const HttpProcessor::Task &t) {
  if (this != &t) {
    m_components = t.m_components;
    m_connection = t.m_connection;
    m_counter = t.m_counter;
    (*m_counter)++;
  }
  return *this;
}

HttpProcessor::Task &HttpProcessor::Task::operator=(HttpProcessor::Task &&t) {
  m_components = std::move(t.m_components);
  m_connection = std::move(t.m_connection);
  m_counter = t.m_counter;
  t.m_counter = nullptr;
Repository: oatpp/oatpp
File: src/oatpp/web/server/HttpConnectionHandler.cpp (C++)

Code before:
  connection->setOutputStreamIOMode(oatpp::data::stream::IOMode::BLOCKING);
  connection->setInputStreamIOMode(oatpp::data::stream::IOMode::BLOCKING);

  /* Create working thread */
  std::thread thread(&HttpProcessor::Task::run, HttpProcessor::Task(m_components, connection, m_spawns));
  
  /* Get hardware concurrency -1 in order to have 1cpu free of workers. */
  v_int32 concurrency = oatpp::concurrency::getHardwareConcurrency();
  if(concurrency > 1) {
    concurrency -= 1;
  }
  
  /* Set thread affinity group CPUs [0..cpu_count - 1]. Leave one cpu free of workers */
  oatpp::concurrency::setThreadAffinityToCpuRange(thread.native_handle(), 0, concurrency - 1 /* -1 because 0-based index */);
  
  thread.detach();

}

void HttpConnectionHandler::stop() {
  /* Wait until all connection-threads are done but no longer than 1min */
  auto startTime = std::chrono::system_clock::now();
  auto timeout = std::chrono::minutes(1);

  while(m_spawns->load() != 0) {
    auto elapsed = std::chrono::system_clock::now() - startTime;
    if(elapsed < timeout) {
      std::this_thread::sleep_for(std::chrono::milliseconds(100));
    } else {
      if (m_spawns->load() > 0) {
        OATPP_LOGW("[oatpp::web::server::HttpConnectionHandler::stop()]", "Timeout while waiting 1 minute for all connections to close. There are %ul connections still alive.", m_spawns->load());
      }
      break;
    }
  }
}

}}}


Review comment (suggestion):
Once `stop()` was called, the `handleConnection()` should not populate any more threads.

Code after:

  if (m_continue.load()) {

    connection->setOutputStreamIOMode(oatpp::data::stream::IOMode::BLOCKING);
    connection->setInputStreamIOMode(oatpp::data::stream::IOMode::BLOCKING);

    /* Create working thread */
    std::thread thread(&HttpProcessor::Task::run, std::move(HttpProcessor::Task(m_components, connection, &m_spawns)));

    /* Get hardware concurrency -1 in order to have 1cpu free of workers. */
    v_int32 concurrency = oatpp::concurrency::getHardwareConcurrency();
    if (concurrency > 1) {
      concurrency -= 1;
    }

    /* Set thread affinity group CPUs [0..cpu_count - 1]. Leave one cpu free of workers */
    oatpp::concurrency::setThreadAffinityToCpuRange(thread.native_handle(),
                                                    0,
                                                    concurrency - 1 /* -1 because 0-based index */);

    thread.detach();
  }

}

void HttpConnectionHandler::stop() {
  m_continue.store(false);

  /* Wait until all connection-threads are done */
  while(m_spawns.load() != 0) {
    std::this_thread::sleep_for(std::chrono::milliseconds(100));
  }
}

}}}
Repository: oatpp/oatpp
File: src/oatpp/web/server/HttpConnectionHandler.cpp (C++)

Code before:
  
  /* Get hardware concurrency -1 in order to have 1cpu free of workers. */
  v_int32 concurrency = oatpp::concurrency::getHardwareConcurrency();
  if(concurrency > 1) {
    concurrency -= 1;
  }
  
  /* Set thread affinity group CPUs [0..cpu_count - 1]. Leave one cpu free of workers */
  oatpp::concurrency::setThreadAffinityToCpuRange(thread.native_handle(), 0, concurrency - 1 /* -1 because 0-based index */);
  
  thread.detach();

}

void HttpConnectionHandler::stop() {
  /* Wait until all connection-threads are done but no longer than 1min */
  auto startTime = std::chrono::system_clock::now();
  auto timeout = std::chrono::minutes(1);

  while(m_spawns->load() != 0) {
    auto elapsed = std::chrono::system_clock::now() - startTime;
    if(elapsed < timeout) {
      std::this_thread::sleep_for(std::chrono::milliseconds(100));
    } else {
      if (m_spawns->load() > 0) {
        OATPP_LOGW("[oatpp::web::server::HttpConnectionHandler::stop()]", "Timeout while waiting 1 minute for all connections to close. There are %ul connections still alive.", m_spawns->load());
      }
      break;
    }
  }
}

}}}


Review comment (suggestion):
let's just block forever - until all threads are done.
Exiting this loop having running server threads will put an application into an unpredictable state.

Code after:

    /* Create working thread */
    std::thread thread(&HttpProcessor::Task::run, std::move(HttpProcessor::Task(m_components, connection, &m_spawns)));

    /* Get hardware concurrency -1 in order to have 1cpu free of workers. */
    v_int32 concurrency = oatpp::concurrency::getHardwareConcurrency();
    if (concurrency > 1) {
      concurrency -= 1;
    }

    /* Set thread affinity group CPUs [0..cpu_count - 1]. Leave one cpu free of workers */
    oatpp::concurrency::setThreadAffinityToCpuRange(thread.native_handle(),
                                                    0,
                                                    concurrency - 1 /* -1 because 0-based index */);

    thread.detach();
  }

}

void HttpConnectionHandler::stop() {
  m_continue.store(false);

  /* Wait until all connection-threads are done */
  while(m_spawns.load() != 0) {
    std::this_thread::sleep_for(std::chrono::milliseconds(100));
  }
}

}}}