Details
-
Task
-
Status: In Review (View Workflow)
-
Minor
-
Resolution: Unresolved
-
None
Description
It is easy to pass wrong arguments in `my_error`: it is vararg, and not printf-like.
One common mistake is passing LEX_CSTRING instead of char*.
1. We can protect from LEX_CSTRING by implementing variadic template guard, forbidding, or implicitly converting some arguments by a sane set of rules, e.g.:
LEX_CSTRING -> char*,
|
anything other -> _error_
|
(TBD)
|
See also midenok's idea here: https://github.com/tempesta-tech/mariadb/issues/325
It will be still possible to pass int instead of char*, etc.
So, additionally, we can parse a format string and convert an argument to a correct parameter, or throw an assertion.
2. The above approach will reveal errors at runtime, and only for tested cases. We can do it in buildtime, although it will not cover all the cases when error code is determined at runtime:
template<int err_code, class... Args> |
int my_error_guard(Argc... args) |
{
|
static_assert(0); // arguments do not fit the prototype for specified error code |
}
|
|
|
template<class... Args> |
int my_error_guard<ER_CODE>(char* a, int b) |
{
|
my_error(ER_CODE, a, b);
|
}
|
|
|
// And so on |
When error code is dynamically resolved, it will fall back to dynamic check described above:
#define my_error(code, ...) \
|
(std::is_const<code>::value \
|
? my_error_guard<code>(__VA_ARGS__) \
|
: my_error(code, ##VA_ARGS))
|
This implementation will break when const, but not constexpr, error code is passed.
Attachments
Issue Links
- is blocked by
-
-
- Closed
-
- relates to
-
-
- Closed
-
Activity
- the list of templates for specific error messages can (should) be generated by comp_err.
- Ideally we could avoid almost all changes to the sources, by using a macro my_error
Another, less C++y option would be to use macros like
#define my_error(X, F, __VA_ARGS__) my_error_ ## X (F, __VA_ARGS__)
|
#define my_error_ER_STD_RANGE_ERROR("Range error: %-.256s in function %s.", F, __VA__ARGS)
|
...
|
and this way the compiler can see the format string and can verify that parameters are correct
serg
>#define my_error(X, F, _VA_ARGS) my_error ## X (F, _VA_ARGS_)
Me and Wlad were also thinking about it.
This way it will stricten the API, for example you couldn't pass error as a non-macro then,
like
my_error(errcode, ....);
|
my_error(flag ? ER_1 : ER_2, ....);
|
But I think I can generate both codes,
- one for C++ (which will even support non-consts and dispatch it correctly),
- and one for plain C (which will stricten the rules, but protect C users as well)
Turns out:
1. we can't generate C-compatible code at all. The error storage is highly distributed and allows dynamic updates, which makes it possible to be used in plugins (additional error codes can be added there)
2. I screwed up with std::is_const – it accepts types, not expressions. In general, there's no way to distinguish constexpr from non-constexpr until c++20.This makes impossible to save to use expressions like
my_error(flag ? ER_1 : ER_2, ....)
|
If we're forcing compile-time type checking.
serg, (1) still doesn't mean we can't do C-compatible wrappers at all. I see following in service_my_print_error.h:
#ifdef MYSQL_DYNAMIC_PLUGIN
|
|
|
#define my_error my_print_error_service->my_error_func
|
#define my_printf_error my_print_error_service->my_printf_error_func
|
#define my_printv_error(A,B,C,D) my_print_error_service->my_printv_error_func(A,B,C,D)
|
|
|
#else
|
extern void my_error(unsigned int nr, unsigned long MyFlags, ...);
|
extern void my_printf_error(unsigned int my_err, const char *format, unsigned long MyFlags, ...);
|
extern void my_printv_error(unsigned int error, const char *format, unsigned long MyFlags,va_list ap);
|
So probably my_error is defined separately for plugins, and for non-plugin case we can collect all the messages in one place, namely errmsg-utf8.txt (currently, error messages are stored in several places more in the code).
serg please review patches on bb-10.5-nikita regarding this ticket