Minor cleanup

[bansan85]

After reading some part of the library, I noticed some minor change with ABI break.

1. unverified_safe_because is template<size_t N> but N is never used.

Another declaration could be:

inline auto unverified_safe_because(const char *reason)

2. const -> static constexpr

The library is explicitly c++17. There is lots of const int CompileErrorCode = 42; that could be replaced by static constexpr int CompileErrorCode = 42;

3. in #define helper_create_converted_field, isFrozen is unused

So new declaration could be:

#define helper_create_converted_field(fieldType, fieldName)

4. Finally a question: the library is explicitly not thread-safe.

So why there is a std::mutex callback_lock, RLBOX_SHARED_LOCK, RLBOX_ACQUIRE_SHARED_GUARD and RLBOX_ACQUIRE_UNIQUE_GUARD? Is this a try to make a thread-safe library?

Thanks,

PS: If you're fine with these changes, I can implement them.

[deian]

These sound good to me (thanks @bansan85 !)

[shravanrn]

unverified_safe_because is template<size_t N> but N is never used.

This is by design. This pattern forces all callers to only pass local strings (character arrays that have not decayed into pointers). If a caller passes in a string whose source is unknown to this API that is a code-smell and likely a mistake. So i would leave this as is.

const -> constexpr

Makes sense, a PR for this would be great! 👍 Many of these don't need to be static though. As a general rule, I think we want to stay away from static unless we explicitly need it. The compiler is going to optimize all of this out either way :)

in #define helper_create_converted_field, isFrozen is unused

This is for backward compatibility with a feature that was removed, which we may be re-introduced in the future. Unfortunately removing this field now will break ABI with existing uses. If you can remove this in a way that does not break the ABI (by adding an overload that discards the extra params), awesome! If not, i think we won't be able to easily make this change.

Finally a question: the library is explicitly not thread-safe.

In general RLBox can be thread-safe. The early prototypes in fact included support for multiple threads. It is in the roadmap to make the library fully thread-safe in the future (about 3/4th of the work for this is already done which are the parts you are seeing).