yhirose / cpp-peglib

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

Replace peg::any with std::any #72

Closed romeoxbm closed 4 years ago

romeoxbm commented 4 years ago

Because of the usage of dynamic_cast in peg::any, it is impossible to use cpp-peglib in projects where RTTI is disabled. This pull request replace peg::any with std::any and update the README accordingly. Changes have been tested with GCC 9.2.1 (with -fno-rtti).

romeoxbm commented 4 years ago

Of course, if approved, this change will require C++17.

yhirose commented 4 years ago

@romeoxbm, thank you for your pull request! But, I still need to support compilers C++11 and C++14 for a while. (Unfortunately, things are moving more slowly than we expect...)

If we want to use std::any for C++17 compilers and avoid breaking build with older compilers, we may take the following approach with the C++17 __has_include macro:

#if defined(__has_include) && __has_include(<any>) 
#include <any>
#endif
...

namesapce peg {
...
#if defined(__has_include) && __has_include(<any>) 
namespace any = std::any;
#else
class any
{
public:
    any() : content_(nullptr) {}
...
#endif

What do you think?

romeoxbm commented 4 years ago

That's exactly what I was thinking when I realized why those pipelines were failing. I'll update the PR as soon as I can today.

yhirose commented 4 years ago

Looks fantastic! The only thing is that the code is using PEGLIB_USE_STD_ANY instead of __has_include. Was there any reason why __has_include?

romeoxbm commented 4 years ago

PEGLIB_USE_STD_ANY is set to 1 when __cplusplus (or _MSVC_LANG when using MSVC) is greater than or equal to 201703L. IMHO it's preferable to perform this kind of check rather than checking if the any include is available. This way, we use std::any when an user is explicitly using the C++17 standard (e.g. using -std=C++17 or /std:C++17), since the any header presence isn't guaranteeing the C++17 standard is in use (it's just another proof that the compiler is C++17 capable).

yhirose commented 4 years ago

Thanks for your reply. I just merged it into the master branch. Thanks for your contribution!

romeoxbm commented 4 years ago

You're welcome