-
Notifications
You must be signed in to change notification settings - Fork 593
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Support for Metadata on Enumerators #2642
Add Support for Metadata on Enumerators #2642
Conversation
@@ -1674,41 +1689,33 @@ enumerator | |||
: ICE_IDENTIFIER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also changed the parsing rule for enumerator
.
For some reason, it's currently returning an EnumeratorList
with one element in it...
I updated the code to just return a EnumeratorPtr
instead.
cpp/src/Slice/Grammar.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all generated code, no need to look at it unless you really want to.
EnumeratorValues.ice:13: value for enumerator `D' is out of range | ||
EnumeratorValues.ice:14: value for enumerator `E' is out of range | ||
EnumeratorValues.ice:15: value for enumerator `F' is out of range | ||
EnumeratorValues.ice:15: enumerator `F' has the same value as enumerator `B' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My changes have a small impact on the ordering/logic we use to check for duplicate enumerators,
hence this small change in the error detection test.
} | ||
| ICE_IDENTIFIER '=' enumerator_initializer | ||
{ | ||
auto ident = dynamic_pointer_cast<StringTok>($1); | ||
auto ens = make_shared<EnumeratorListTok>(); | ||
ContainerPtr cont = currentUnit->currentContainer(); | ||
EnumPtr cont = dynamic_pointer_cast<Enum>(currentUnit->currentContainer()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't check that cont is not null because an enumerator can only be defined in an Enum container?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, by the time we hit this parsing rule, if there isn't a container, something has already gone horribly wrong.
} | ||
auto enumeratorList = make_shared<EnumeratorListTok>(); | ||
enumeratorList->v.push_front(enumerator); | ||
$$ = enumeratorList; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not totally clear to me why we have the %empty rule. Presumably the 'meta_data enumerator' rule above is sufficient to terminate the parsing of a non-empty enumeration.
According to:
https://doc.zeroc.com/ice/3.7/the-slice-language/user-defined-types/enumerations
Slice does not permit empty enumerations.
Is this a bug and the parser actually allows empty enumerations?
In new Slice:
https://docs.icerpc.dev/slice2/language-guide/enum-types
A checked enumeration must have at least one enumerator, while an unchecked enumeration may have no enumerator at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a bug
nope, ...
and the parser actually allows empty enumerations?
... and yes!
You're correct, empty enums are disallowed in Slice, but we still have the logic to parse them correctly.
If we didn't have this rule, then an empty enum would cause a syntax error: expected identifier, '[', ...
to be emitted:
A) this error message isn't super helpful
B) and it isn't completely accurate, an empty enum is a semantic error, not a syntax error.
C) The bison compiler has no syntax-error recovery, so we only emit true syntax errors as a last resort.
So, the idea is, we allow this at the parser level, then later on we perform the check (where we use this grammar rule). This way we can gracefully handle the error, and provide a more helpful error message. See:
Lines 1623 to 1636 in 6980c56
'{' enumerator_list '}' | |
{ | |
auto en = dynamic_pointer_cast<Enum>($2); | |
if (en) | |
{ | |
auto enumerators = dynamic_pointer_cast<EnumeratorListTok>($4); | |
if (enumerators->v.empty()) | |
{ | |
currentUnit->error("enum `" + en->name() + "' must have at least one enumerator"); | |
} | |
currentUnit->popContainer(); | |
} | |
$$ = $2; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as a note, if you ever look through the grammar you'll actually see alot of this.
'bugs' where the parser accepts completely bogus Slice!
For the reason that if we don't, we'll get Bison's default error message, and parsing halts immediately; both not great.
So, we allow the bogus Slice for a little bit until we hit a place where we can safely reject it with a custom error message.
} | ||
else | ||
{ | ||
$$ = cont->createEnumerator(ident->v, nullopt); // Dummy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the dummy comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case only occurs when the cast auto intVal = dynamic_pointer_cast<IntegerTok>($3)
fails.
But this grammar rule is only triggered when there's an integer literal present...
So something must of gone wrong for the cast to fail. To be honest, I didn't look too closely at this specific case,
but it might just be impossible to end up in this case after thinking about it now.
} | ||
| ICE_IDENTIFIER '=' enumerator_initializer | ||
{ | ||
auto ident = dynamic_pointer_cast<StringTok>($1); | ||
auto ens = make_shared<EnumeratorListTok>(); | ||
ContainerPtr cont = currentUnit->currentContainer(); | ||
EnumPtr cont = dynamic_pointer_cast<Enum>(currentUnit->currentContainer()); | ||
auto intVal = dynamic_pointer_cast<IntegerTok>($3); | ||
if (intVal) | ||
{ | ||
if (intVal->v < 0 || intVal->v > std::numeric_limits<int32_t>::max()) | ||
{ | ||
currentUnit->error("value for enumerator `" + ident->v + "' is out of range"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens after you issue this error?
My understanding is the existing code tried to keep going.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the new code also keeps going. On the line right under this we set:
$$ = cont->createEnumerator(ident->v, static_cast<int>(intVal->v));
this is effectively a return statement in Bison. So we still construct the enumerator, and return it.
@@ -896,8 +893,7 @@ namespace Slice | |||
class Enumerator final : public virtual Contained | |||
{ | |||
public: | |||
Enumerator(const ContainerPtr&, const std::string&, std::optional<int>); | |||
void init() final; | |||
Enumerator(const ContainerPtr&, const std::string&, int, bool); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please give names to parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also find the previous std::optional<int>
nicer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please give names to parameters.
Alright, I'll do that in a follow up PR.
None of the grammar headers use named parameters. I have no idea why, that's just how it always was.
I also find the previous std::optional nicer
It doesn't really work here though.
It's probably hard to follow without names, but take:
enum Foo { A }
Here the enumerator has a name A
, a value of 0
, and hasExplicitValue
(the bool) is false
.
With an optional
, there's no way to still provide a value in the case when it's 'false
' (ie. empty).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I didn't realize the caller provides the non-explicit value. Seems like an odd choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's just more straightforward this way. Enumerator
has always held it's actual value (explicit or implicit), since often, that's all the code gen cares about. A key fact is that Enumerator
on it's own cannot compute an implicit value for itself.
Only Enum
has enough information to compute that.
The old logic was Enum::createEnumerator
would set a special value of -1
if none was provided.
And the constructor Enumerator::Enumerator
would call back into Enum::newEnumerator
to check for this special value and patch it with a real value.
But... there's no benefit to delaying this, instead of just computing the value up front. Enum
already has all the necessary information. No need to jump back and forth between Enum
and Enumerator
, and this avoids storing a special 'replace me' value of -1
too.
This PR adds support for metadata on enumerators, and simplifies the code for constructing enumerators (#2515):
Currently,
createEnumerator
andvalidateEnumerator
are onContainer
, notEnum
.And
Enum
has it's own function namednewEnumerator
.All 3 of these functions were simplified and merged into a single
createEnumerator
onEnum
(like you'd expect).