-
Notifications
You must be signed in to change notification settings - Fork 49
explicit_object_extensions provisional -- SIMICS-23313
#399
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
base: main
Are you sure you want to change the base?
Conversation
bcd632e to
aa03516
Compare
| # extension status: | ||
| # None -> dual extension and decl | ||
| # True -> extension | ||
| # False -> explicit decl |
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 ternary rep is pretty damn ugly. @mandolaerik does this deserve the use of enum (which'd be a fresh DMLC dependency?) Or is there a third approach perhaps I haven't thought of?
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.
The current rep is fine, but I also wouldn't object to enum. The question it poses to me is rather whether it makes sense to handle subobj merge in this incremental pairwise fashion -- I think this logic would be much easier to express if we just merged all object defs at once.
IIRC the pairwise thing is there for a historical reason: before dml 1.4, templates would consist of pre-merged subobj defs, so a template used many times did not have to merge defs again.
It would probably make sense to refactor all of merge_subobj_defs, but no need to do within this PR.
| fmt = ("object '%s' not declared elsewhere." | ||
| " To declare and define a new object, omit 'in'.") | ||
|
|
||
| class EMULTIOBJDECL(DMLError): |
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 struggled a bit with naming of these error tags, but ultimately it doesn't matter much.
|
PR Verification: ✅ success |
|
@mandolaerik ping |
| 'object : REGISTER objident array_list sizespec offsetspec maybe_istemplate object_spec' | ||
| t[0] = ast.object_(site(t), t[2], 'register', t[3], | ||
| t[4] + t[5] + t[6] + t[7]) | ||
| 'object : maybe_extension REGISTER objident array_list sizespec offsetspec maybe_istemplate object_spec' |
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.
do we really want to allow in register x size 4 @ 4711 { } ? all the special syntax is a clear declaration marker to me.
I suggest in KIND NAME [INDICES] { ... } as the only allowed form, analogous to the grammar for in each xyz { ... }
BTW: should we forbid the [_ < ...] index forms outside extensions when the provisional is active?
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.
do we really want to allow in register x size 4 @ 4711 { } ?
I reference this in the JIRA issue. I intentionally made this feature as consistent as possible, and the questions of what we should or should not allow a separate issue -- because the answers are not obvious, (in particular, although I previously agreed with forbidding [_ < ...] as a decl, upon reevaluation it's less obvious to me that such a decl would be nonsensical.)
In other words: i don't care when it comes to this PR. To answer these questions, we need deeper discussion and analysis, and this PR has no need for that since this is an unstable provisional. Besides, the consistent design is a virtue in its own right, and answers about it vs. any ad-hoc restrictions we make could perhaps be enlightened by Gustav's use of it.
| @feature | ||
| class explicit_object_extensions(ProvisionalFeature): | ||
| '''<a id="explicit_object_extensions"/> | ||
| This feature extends the DML syntax for object declarations to distinguish |
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.
newline after <a, otherwise markdown is messed up in the first paragraph.
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.
wait-- you mean
<a
id="explicit_object_extensions"/>
? That's messed up, man...
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.
wait i can't see that reflected for explicit_param_decls. Does that mean its docs are messed up?
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.
... in fact I can't see a <a-newline pattern being used anywhere?
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.
no, I meant to write:
<a id="foo"/>
blah `blah`
instead of:
<a id="foo"/>
blah `blah`
The first one is rendered like this:
blah blah
The second one is rendered like this:
blah `blah`and don't ask me why everything is underlined now
| " To declare and define a new object, omit 'in'.") | ||
|
|
||
| class EMULTIOBJDECL(DMLError): | ||
| """When the `explicit_object_extensions` provisional feature is enabled, |
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.
link
|
|
||
|
|
||
| class EEXTENSION(DMLError): | ||
| """When the `explict_object_extensions` provisional feature is enabled, |
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.
link
No description provided.