Отсылаю код на ревизию Сионисту

Модераторы: Hawk, Romeo, Absurd, DeeJayC, WinMain

Ответить
Absurd
Сообщения: 1228
Зарегистрирован: 26 фев 2004, 13:24
Откуда: Pietari, Venäjä
Контактная информация:

Нужно как-то создать тред в Win32. Вообще-то это достаточно нетривиальная задача. У меня сие скомпилировалось раза с третьего. Нужно просунуть указатель на объект-переходник в __stdcall функцию, скастовать ее в полиморфный тип и дернуть у него виртуальный метод. В результате получился все равно сырейший код. До реальных проектов этому коду как до луны раком. Поэтому умные люди и придумали класс std::thread.

Но мне нравится как Сионист делает ревизию кода. Интересно какие он тут найдет ошибки.

Код: Выделить всё

#include <memory>
#include <string>
#include <iostream>
#include <Windows.h>
#include <process.h>
#include <stdexcept>
#include <tuple>
#include <atomic>

class MyThread {
  HANDLE handle_;
  unsigned tid_;
  bool detached_;
  struct TypeErasure {
    virtual unsigned run(unsigned tid) = 0;
    virtual ~TypeErasure() = default;
  };
  std::unique_ptr<TypeErasure> runnable_;
  template<typename ThreadRoutine> struct Thunc : public TypeErasure {
    ThreadRoutine tr_;
    Thunc(ThreadRoutine tr)
      :tr_(tr)
    {}
    unsigned run(unsigned tid) override
    {
      return tr_(tid);
    }
  };
  static unsigned __stdcall entry(void* arg)
  {
    MyThread* cookie = static_cast<MyThread*>(arg);
    return cookie->runnable_->run(cookie->tid_);
  }
public:
  template<typename ThreadRoutine> MyThread(ThreadRoutine tr)
    : runnable_(std::make_unique< MyThread::Thunc<ThreadRoutine> >(tr))
    , detached_(false)
  {
    handle_ = (HANDLE)_beginthreadex(NULL, 0, &entry, this, CREATE_SUSPENDED, &tid_);
  }

  void operator()()
  {
    ResumeThread(handle_);
  }

  void detach()
  {
    detached_ = true;
  }

  std::tuple<unsigned, bool> join(DWORD millis = INFINITE)
  {
    if (!detached_) {
      DWORD waitResult = WaitForSingleObject(handle_, millis);
      if (waitResult == WAIT_OBJECT_0) {
        DWORD exitCode = 0;
        if (GetExitCodeThread(handle_, &exitCode)) {
          return std::make_tuple(exitCode, true);
        } else {
          return std::make_tuple(-1, false);
        }
      } else {
        return std::make_tuple(-1, false);
      }
    } else {
      throw std::runtime_error("Attempt to join a detached thread");
    }
  }

  ~MyThread()
  {
    if (!detached_) {
      bool joinSucc = false;
      try {
        std::tie(std::ignore, joinSucc) = join(0);
      } catch (...) { }
      if (!joinSucc) {
        std::terminate();
      }
    }
  }
};

namespace {
std::atomic_flag own_io = ATOMIC_FLAG_INIT;
}

unsigned func(unsigned tid)
{
  for (int i = 0; i < 100; ++i) {
    while (own_io.test_and_set(std::memory_order_acquire)) {
      Sleep(0);
    }
    std::cout << "ThreadID {" << std::hex << tid << "} say " << std::dec << i << "\n";
    own_io.clear(std::memory_order_release);
  }
  return 0;
}

int main()
{
  MyThread thread1(func), thread2(func), thread3(func);
  thread1();
  thread2();
  thread3();
  thread1.join();
  thread2.join();
  thread3.join();
  return 0;
}
2B OR NOT(2B) = FF
Absurd
Сообщения: 1228
Зарегистрирован: 26 фев 2004, 13:24
Откуда: Pietari, Venäjä
Контактная информация:

Кстати, есть очень и очень серьезная и вполне очевидная.
2B OR NOT(2B) = FF
Аватара пользователя
Сионист
Сообщения: 1211
Зарегистрирован: 31 мар 2014, 06:18

||=== Build failed: 50 error(s), 0 warning(s) (0 minute(s), 2 second(s)) ===|
Чего, кстати, и следовало ожидать от затеи "Не удобно писать класс поток и передавать в функцию потока this, поэтому "умные" люди придумали именно так и делать".
Писать можно на чём угодно, но зачем же так себя ограничивать? Пиши на c.
Absurd
Сообщения: 1228
Зарегистрирован: 26 фев 2004, 13:24
Откуда: Pietari, Venäjä
Контактная информация:

А что за ошибки ? Я вроде свежака не нагородил особо. Кинь полный дамп. На Visual С++ 2015 собирается.
2B OR NOT(2B) = FF
Аватара пользователя
Сионист
Сообщения: 1211
Зарегистрирован: 31 мар 2014, 06:18

Вся ругань компилятора не лезет в ограничение по длине поста. Кстати, что такое tr_? И обратите внимание:
1. Все локальные данные потока локальны безо всяких выкрутасов просто потому, что это функция.
2. К любой глобальной переменной можно обращаться также, как и из обычной функции. Добавляется лишь синхронизация, но само обращение не меняется вообще и не требует передачи this и складывания всего глобала в специальный класс.
3. Любые мьютексы и критикальные секции можно отлично объединить в классы, или структуры с самими глобальными переменными.
4. Если необходимо передать функции потока параметры, то можно сложить в класс, структуру, или даже массив именно их и передать указатель на эту сущность, только синтаксически он оформляется как сырой.
Так нафига ж городить всеобъемлющий класс аля джава? Вы для каждой обычной функции его тоже городите? Неужели нет? А чем же потоковая то провинилась?
Писать можно на чём угодно, но зачем же так себя ограничивать? Пиши на c.
Absurd
Сообщения: 1228
Зарегистрирован: 26 фев 2004, 13:24
Откуда: Pietari, Venäjä
Контактная информация:

$ g++ -std=c++14 -Wall -Wextra -Weffc++ -pedantic ./threads.cpp
./threads.cpp:10:7: warning: 'class MyThread' has pointer data members [-Weffc++]
class MyThread {
^
./threads.cpp:10:7: warning: but does not override 'MyThread(const MyThread&)' [-Weffc++]
./threads.cpp:10:7: warning: or 'operator=(const MyThread&)' [-Weffc++]
./threads.cpp: In instantiation of 'MyThread::MyThread(ThreadRoutine) [with ThreadRoutine = unsigned int (*)(unsigned int)]':
./threads.cpp:103:24: required from here
./threads.cpp:18:32: warning: 'MyThread::runnable_' will be initialized after [-Wreorder]
std::unique_ptr<TypeErasure> runnable_;
^
./threads.cpp:13:8: warning: 'bool MyThread::detached_' [-Wreorder]
bool detached_;
^
./threads.cpp:35:36: warning: when initialized here [-Wreorder]
template<typename ThreadRoutine> MyThread(ThreadRoutine tr)
^
./threads.cpp:35:36: warning: 'MyThread::handle_' should be initialized in the member initialization list [-Weffc++]
./threads.cpp:35:36: warning: 'MyThread::tid_' should be initialized in the member initialization list [-Weffc++]
Ну хз, gcc более суровый конечно, но у меня скомпилировалось с такими варнингами. Это все очень серьезно, конечно, но это не несколько экранов.
PS: не знал что make_unique добавили только в новый 14-й стандарт. Я думал оно старше.
2B OR NOT(2B) = FF
Absurd
Сообщения: 1228
Зарегистрирован: 26 фев 2004, 13:24
Откуда: Pietari, Venäjä
Контактная информация:

Код: Выделить всё

#include <memory>
#include <string>
#include <iostream>
#include <windows.h>
#include <process.h>
#include <stdexcept>
#include <tuple>
#include <atomic>

namespace detail {
static unsigned __stdcall entry(void* arg);

struct ThunkBase {
  virtual unsigned run(unsigned tid) = 0;
  virtual ~ThunkBase() = default;
};

template<typename ThreadRoutine>
struct Thunk : public ThunkBase {
  ThreadRoutine threadRoutine_;
  Thunk(ThreadRoutine tr) 
    : threadRoutine_(tr)
  {}
  unsigned run(unsigned tid) override
  {
    return threadRoutine_(tid);
  }
};

struct ThreadHandleWrapper {
  HANDLE handle_;
  std::unique_ptr<ThunkBase> thunk_;
  unsigned tid_;
  DWORD threadExitCode_;
  bool threadStarted_;
  bool cleanThreadExit_;

  ThreadHandleWrapper()
    : handle_(nullptr)
    , thunk_(nullptr)
    , tid_(0)
    , threadExitCode_(-1)
    , threadStarted_(false)
    , cleanThreadExit_(false)
  {}

  ThreadHandleWrapper(const ThreadHandleWrapper&) = delete;
  ThreadHandleWrapper(ThreadHandleWrapper&&) = delete;

  ThreadHandleWrapper& operator=(const ThreadHandleWrapper&) = delete;
  ThreadHandleWrapper& operator=(ThreadHandleWrapper&&) = delete;

  template<typename ConcreteThreadRoutine>
  void postInit(ConcreteThreadRoutine ctr)
  {
    thunk_ = std::unique_ptr< ThunkBase >(new Thunk < ConcreteThreadRoutine >(ctr));
    handle_ = reinterpret_cast<HANDLE>(_beginthreadex(nullptr, 0, &detail::entry, this, CREATE_SUSPENDED, &tid_));
    if (!handle_) {
      throw std::runtime_error("Can't spawn a thread");
    }
  }

  void startThread()
  {
    ResumeThread(handle_);
    threadStarted_ = true;
  }

  std::tuple<unsigned, bool> join(DWORD millis = INFINITE)
  {
    if (!cleanThreadExit_) {
      DWORD waitResult = WaitForSingleObject(handle_, millis);
      if (waitResult == WAIT_OBJECT_0) {
        if (GetExitCodeThread(handle_, &threadExitCode_)) {
          CloseHandle(handle_);
          handle_ = nullptr;
          cleanThreadExit_ = true;
          return std::make_tuple(threadExitCode_, true);
        } else {
          return std::make_tuple(-1, false);
        }
      } else {
        return std::make_tuple(-1, false);
      }
    } else {
      return std::make_tuple(threadExitCode_, true);
    }
  }

  ~ThreadHandleWrapper()
  {
    if (threadStarted_) {
      bool joinSucc = false;
      try {
        std::tie(std::ignore, joinSucc) = join(0);
      } catch (...) {}
      if (!joinSucc) {
        std::terminate();
      }
    }
  }
};

static unsigned __stdcall entry(void* arg)
{
  ThreadHandleWrapper* cookie = static_cast<ThreadHandleWrapper*>(arg);
  return cookie->thunk_->run(cookie->tid_);
}
}

class MyThread {
  std::unique_ptr<detail::ThreadHandleWrapper> handleWrapper_;
public:
  MyThread()
	:handleWrapper_()
  {}

  template<typename ThreadRoutine>
  explicit MyThread(ThreadRoutine tr)
    : handleWrapper_(new detail::ThreadHandleWrapper)
  {
    handleWrapper_->postInit<ThreadRoutine>(tr);
  }

  MyThread(const MyThread&) = delete;
  MyThread(MyThread&& other)
    :handleWrapper_(std::move(other.handleWrapper_))
  {}

  MyThread& operator=(const MyThread&) = delete;
  MyThread& operator=(MyThread&& other)
  {
    handleWrapper_ = std::move(other.handleWrapper_);
    return *this;
  }

  void operator()()
  {
    if (handleWrapper_) {
      handleWrapper_->startThread();
    } else {
      throw std::runtime_error("Not initialized");
    }
  }

  std::tuple<unsigned, bool> join(DWORD millis = INFINITE)
  {
    if (handleWrapper_) {
      return handleWrapper_->join(millis);
    } else {
      throw std::runtime_error("Not initialized");
    }
  }
};

namespace {
std::atomic_flag own_io = ATOMIC_FLAG_INIT;
}

unsigned func(unsigned tid)
{
  for (int i = 0; i < 100; ++i) {
    while (own_io.test_and_set(std::memory_order_acquire)) {
      Sleep(0);
    }
    std::cout << "ThreadID {" << std::hex << tid << "} say " << std::dec << i << "\n";
    own_io.clear(std::memory_order_release);
  }
  return 0;
}

int main()
{
  MyThread threads[10];
  for (std::size_t i = 0; i < sizeof(threads) / sizeof(threads[0]); ++i) {
    threads[i] = MyThread(func);
  }
  for (std::size_t i = 0; i < sizeof(threads) / sizeof(threads[0]); ++i) {
    threads[i]();
  }
  for (std::size_t i = 0; i < sizeof(threads) / sizeof(threads[0]); ++i) {
    threads[i].join(INFINITE);
  }
  return 0;
}
Ну там поставил Mingw, поколдовал немного,

Код: Выделить всё

00:01:20 **** Rebuild of configuration Default for project cdt1 ****
Info: Internal Builder is used for build
g++ -std=c++1y "-IF:\\cpp\\msys2\\mingw64\\include\\c++\\5.2.0" -O2 -g -pedantic -Wall -Wextra -Weffc++ -c -fmessage-length=0 -o cdt1.o "..\\cdt1.cpp" 
g++ -o cdt1 cdt1.o 

00:01:23 Build Finished (took 2s.859ms)
2B OR NOT(2B) = FF
Absurd
Сообщения: 1228
Зарегистрирован: 26 фев 2004, 13:24
Откуда: Pietari, Venäjä
Контактная информация:

1. Все локальные данные потока локальны безо всяких выкрутасов просто потому, что это функция.
Функцией в С++ является все то для чего определен operator(). Но я не могу передать в _beginthreadex(), например, лямбду вида [this](unsigned tid) -> unsigned { ... }. Поэтому нужен универсальный конвертор интерфейса все что угодно с сигнатурой unsigned(T...) в unsigned (__stdcall*)(void*).
К любой глобальной переменной можно обращаться также, как и из обычной функции. Добавляется лишь синхронизация, но само обращение не меняется вообще и не требует передачи this и складывания всего глобала в специальный класс.
Тем не менее, контекст для треда сподручнее и надежнее создать там где тред создается. Просовывать же его через void* это уродство. Небезопасное, причем.
Любые мьютексы и критикальные секции можно отлично объединить в классы, или структуры с самими глобальными переменными.
Немасштабируемые ресурсы типа мутексов лучше делать локальными. Блокировать на уровне объекта, а не на уровне всего класса или глобальной структуры.
Так нафига ж городить всеобъемлющий класс аля джава? Вы для каждой обычной функции его тоже городите?
Сказал человек который уныло насилует iostream чтобы сделать из него boost::locale.
Это не всеобъемлющий класс. Это конвертор Си-интерфейса к С++-интерфейсу где помимо простых функций есть еще и функциональные объекты. Он адаптирует системный апишный вызов к произвольной С++-ной сущности имеющей подходящую сигнатуру вызова.

В соответствии с правилами хорошего проектирования один класс должен инкапсулировать одну сущность, делать это филигранно, и иметь минимальный, но достаточный интерфейс для работы с инкупсулированной сущностью. Не могу сказать что этот объект работает филигранно т.к. в примитивах такого рода баги и косяки всплывают годами, но он инкапсулирует одну сущность, а именно системный хендл треда. Интерфейс у него минимальный - создать, запустить, передать владение, дождаться завершения и получить код возврата. Но не достаточный, нужно еще реализовать detach() который передавал бы владение тредом глобальному деинициализатору для того чтобы делать TerminateThread() в atexit().

Класс java.lang.Thread всеобъемлющим не является. То что от него можно унаследоваться есть рудимент моды на ООП и строительство иерархий классов в контексте которой Джава и возникла. Но более предпочтительный и современный вариант это применить агрегирование вместо наследования и передать инстансу треда интерфейс Runnable. В таком случае класс Thread будет просто враппеером вокруг системного треда либо внутреннего шедулера JVM если платформа не поддерживает треды, либо поддерживает их не таким образом как того хочет спецификация JVM. То есть, java.lang.Thread инкапсулирует одну сущность, хотя и имеет некоторый deprecated функционал для обратной совместимости. За филигранность отвечают серьезные люди из Oracle, IBM, Google и Apache Foundation.
2B OR NOT(2B) = FF
Ответить