yhirose / cpp-peglib

A single file C++ header-only PEG (Parsing Expression Grammars) library
MIT License
916 stars 113 forks source link

Avoid requiring pthread on Linux distribution #255

Closed ptal closed 2 years ago

ptal commented 2 years ago

Hi,

I ran into problems trying this library on Ubuntu, and even after linking to pthread I couldn't run my example :( Since you are not using threads anyways, I thought we could eliminate the problem by simply redefining (a simpler version of) call_once and once_flag. I removed the warning in the README, and the pthread thing in the CMake.

Besides, I've added some flags to allow developers importing your library through CMake to disable the builds of example and lint. There was also a warning on CMake version > 3.24 that required a fix.

ptal commented 2 years ago

Seems like the test Calculator_test_with_combinators_and_AST doesn't pass on AppVeyor. Not sure why :confused: It passes on my machine (Ubuntu 20.04, g++ 10.3.0)

yhirose commented 2 years ago

@ptal, thanks for the pull request, but I am not going to accept it since your once_flag isn't protected and breaks the thread safety...

https://codereview.stackexchange.com/questions/117468/stdonce-flag-and-stdcall-once-implementation

ptal commented 2 years ago

It's not thread safe, but nothing is thread safe in your code, right? I mean, you are not using threads :question:

yhirose commented 2 years ago

If a parser client instance is used by more than one threads, then the issue will arise.

mingodad commented 2 years ago

Shouldn't this be a documented feature and the user that want to use it in multiple threads be advised to protect it somehow ?

yhirose commented 2 years ago

Right now, all the internal cached objects in a parser instance are protected with std::once_flag with mutex or atomic variable. So a user doesn't need to worry about the thread safety matter at all.

We could remove it, then a user is responsible to guard method calls of a peg parser when it's called more than one threads. But it's really problematic since users can easily forget the fact... So I am not planning to remove std::call_once and std::once_flag. Thanks for your understanding!

mingodad commented 2 years ago

How about add a conditional macro around std::once_flag that is active by default but users that for some reason prefer not having it/link with pthreads could add it to the command line:

g++ -DCPP_PEGLIB_NO_THREADS *
ptal commented 2 years ago

Right now, all the internal cached objects in a parser instance are protected with std::once_flag with mutex or atomic variable. So a user doesn't need to worry about the thread safety matter at all.

We could remove it, then a user is responsible to guard method calls of a peg parser when it's called more than one threads. But it's really problematic since users can easily forget the fact... So I am not planning to remove std::call_once and std::once_flag. Thanks for your understanding!

Right, I didn't think about the use case where a user calls the parser from different threads, and certainly not that your code was designed for it. Good job ;-)