Added range check of error codes #4249
Conversation
lib/error_catalog.h
Outdated
| /// @param errorCode - integer value for the error/warning | ||
| /// @param name - name for the error. Used to enable/disable all errors of that type | ||
| /// @param forceReplace - override an existing error type in the catalog | ||
| bool add(int errorCode, const char *name, bool forceReplace = false) { | ||
| bool add(MessageType type, int errorCode, const char *name, bool forceReplace = false) { |
There was a problem hiding this comment.
These all appear like compile-time constants. Maybe it is possible to make this a compile-time range check using constexpr?
There was a problem hiding this comment.
It would require actually calling add in compile-time context. However map::emplace is not constexpr (not even in C++23). Or it would need to be a template, somewhat like this:
template<MessageType type, int errCode>
bool add(const char *name, bool forceReplace = flase) {
static_assert(type != MessageType::Error ||
(errorCode >= ErrorType::ERR_MIN_BACKEND && errorCode < ErrorType::ERR_MAX),
"Error code out of range");
...
}That would work too (I believe). It would be called like this:
errorCatalog.add<MessageType::Error, Err::ERR_FOO>("foo");Which is not super-nice, but I'd say it is worth the added safety.
For this to work, the constants would need to be declared in the .h file. Probably as
static constexpr int ERR_MIN_BACKEND = ...;There was a problem hiding this comment.
I'll let you decide whether you consider it worth implementing. I think it'd be nice, since runtime BUGs are a persistence source of annoyance.
There was a problem hiding this comment.
Thanks for your suggestions, they really make sense!
lib/error_catalog.h
Outdated
| /// @param errorCode - integer value for the error/warning | ||
| /// @param name - name for the error. Used to enable/disable all errors of that type | ||
| /// @param forceReplace - override an existing error type in the catalog | ||
| bool add(int errorCode, const char *name, bool forceReplace = false) { | ||
| bool add(MessageType type, int errorCode, const char *name, bool forceReplace = false) { |
There was a problem hiding this comment.
I'll let you decide whether you consider it worth implementing. I think it'd be nice, since runtime BUGs are a persistence source of annoyance.
dmatousek
force-pushed
the
error-catalog-check
branch
from
25cbd2f
to
73e4e43
Compare
dmatousek
force-pushed
the
error-catalog-check
branch
from
73e4e43
to
bf117bf
Compare
| bool add(int errorCode, const char *name, bool forceReplace = false) { | ||
| template <MessageType type, int errorCode> | ||
| bool add(const char *name, bool forceReplace = false) { | ||
| static_assert(type != MessageType::Error || |
There was a problem hiding this comment.
Neat, does this work errorcode or should we make explicit that it is a constexpr? Same for all the error codes.
There was a problem hiding this comment.
Sorry, what do you mean?
There was a problem hiding this comment.
All the integers are const not constexpr. Does this work with static_assert? Or does the compiler convert them automatically to compile time constants?
There was a problem hiding this comment.
Yes, it works. The compilation fails with:
lib/error_catalog.h:112:50: error: static assertion failed
112 | static_assert(type != MessageType::Error ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
113 | (errorCode >= ErrorType::ERR_MIN_BACKEND && errorCode <= ErrorType::ERR_MAX));
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I believe that for an integral type const implies consexpr.
|
What do I have to do to be able to merge p4c PRs? |
|
I think we need to add you as a committer. Working on that. I can merge it for you. |
|
Thank you! |
|
There should be a person at Intel that can add you to p4lang, is that not the case anymore? |
* Added range check of error codes * Added INFO_PROGRESS error code * Fixed isError range * Made range checks compile-time
The PR adds named constants for min and max values of error, warning, and info message codes to be used in backends. Also, the
ErrorCatalog::addmethod now expects a code type so that it can perform range check. E.g.:ErrorCatalog::getCatalog().add<ErrorMessage::MessageType::Info, INFO_USER_CODE>("user-code");If the range check fails, a bug is triggered.
In addition, a new info code INFO_PROGRESS is added.