Fix problem with action parameters with a template type of bool #931
Conversation
| // Currently _Bool is the only one of these oddities but others could be handled here if they arise. | ||
| auto pos = type_name.find("_Bool"); | ||
| if (pos != std::string::npos) { | ||
| type_name.replace(pos, 5, "bool"); |
There was a problem hiding this comment.
Is this 5 is actually std::string("_Bool").size() ?
Also, I assume you expect that whole string should be equal to "_Bool" and not somewhere in the middle.
If so implementation could be like that:
static const std::string _Bool = "_Bool"; if (type_name == _Bool){ type_name.replace(0, _Bool.size(), "bool");
also I'm not sure why are you using replace, is it faster comparing to operator assignment? I assume it should have same performance
There was a problem hiding this comment.
Replace is needed because _Bool is not the whole string. This change was specifically to handle cases like
std::optional<_Bool> which should be converted to std::optional<bool>
There was a problem hiding this comment.
Expanding on the question: what will this do with std::optional<My_Bool>?
There was a problem hiding this comment.
Or, std::optional<MyNS::_Bool>?
C++ types don't meet the requirements of a regular grammar. i.e. neither simple string ops or even regular expressions can disambiguate them.
There was a problem hiding this comment.
@jeffreyssmith2nd got it. do you mind to replace hardcoded 5 with _Bool.size() ?
There was a problem hiding this comment.
C++ types don't meet the requirements of a regular grammar. i.e. neither simple string ops or even regular expressions can disambiguate them.
@tbfleming Certainly, and that's challenge of some of the string based stuff we do in abi/codegen. In this case, I could add a regex to handle some common scenarions (right after a <, comma, or start of a line, etc) which I think would be worth doing to handle cases like what you showed, but I'm not sure there's a solution that handles everything perfectly.
There was a problem hiding this comment.
As long as it's string based, there's a tradeoff between:
- Not recognizing some cases, e.g.
binary_extension<optional<vector<bool>>> - Improperly transforming cases which shouldn't match
Saying abigen doesn't recognize specific cases is easier than saying it scrambles certain user-defined types.
There was a problem hiding this comment.
Especially since working contract code could currently exist which this breaks.
There was a problem hiding this comment.
_Bool only appears because policy.Bool is false.
There was a problem hiding this comment.
Good find, that's probably the right way to fix this.
Change Description
Handle action parameters that have a template type that translates to
_Boolduring codegen.Fixes #862.
API Changes
Documentation Additions