Make override=1 and pure=1 imply virtual=1 to simplify MethodDecl
Categories
(Core :: IPC, enhancement, P3)
Tracking
()
People
(Reporter: cpeterson, Assigned: cpeterson)
References
Details
Attachments
(3 files)
This bug is follow-up ipdl cleanup from bug 1428535 comment 6: 1. Make construction of `MethodDecl` not terrible, so we can specify `override=1` at construction time and have that imply `virtual`, rather than having to specify both. 2. And `pure=1` would work similarly. 3. Maybe we could even consolidate multiple keyword args into an enum as you suggest.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•6 years ago
|
||
While neither MethodDecl inline nor never_inline are set, force_inline is set. However, the force_inline name is misleading because is used to split class declarations and definitions, not to emit `MOZ_ALWAYS_INLINE` in the generated code.
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8942560 [details] Bug 1428984 - Part 1: Consolidate virtual, pure, override, and static MethodDecl types into an MethodSpec enum. https://reviewboard.mozilla.org/r/212812/#review218880 The idea of `MethodSpec` is better than the proliferation of keywords we have for some of these classes. It's not great that we now have invalid values that can be passed into things like `FunctionDecl` or `DestructorDecl`...but I think on balance we're probably better off than we were before.
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8942561 [details] Bug 1428984 - Part 2: Remove unused never_inline flag. https://reviewboard.mozilla.org/r/212814/#review218886
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8942562 [details] Bug 1428984 - Part 3: Remove unused inline flag. https://reviewboard.mozilla.org/r/212816/#review218888
Pushed by cpeterson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/612150969113 Part 1: Consolidate virtual, pure, override, and static MethodDecl types into an MethodSpec enum. r=froydnj https://hg.mozilla.org/integration/autoland/rev/18fb324726e7 Part 2: Remove unused never_inline flag. r=froydnj https://hg.mozilla.org/integration/autoland/rev/1f3edc2b0da5 Part 3: Remove unused inline flag. r=froydnj
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/612150969113 https://hg.mozilla.org/mozilla-central/rev/18fb324726e7 https://hg.mozilla.org/mozilla-central/rev/1f3edc2b0da5
Comment 10•5 years ago
•
|
||
This chunk in ipc/ipdl/ipdl/lower.py makes us output a definition for methods that are marked as pure:
@@ -4887,19 +4893,17 @@ methodDefns."""
cls.stmts[i] = StmtDecl(decl)
defns.addstmts([ defn, Whitespace.NL ])
return cls, defns
def _splitMethodDefn(md, clsname):
saveddecl = deepcopy(md.decl)
md.decl.name = (clsname +'::'+ md.decl.name)
- md.decl.virtual = 0
- md.decl.override = 0
- md.decl.static = 0
+ md.decl.methodspec = MethodSpec.NONE
Note that pure was not set to 0 before, but methodspec is always set to MethodSpec.NONE now. visitMethodDefn checks for MethodSpec.PURE to decide whether to output a definition.
Was that change in behaviour intended?
Assignee | ||
Comment 11•5 years ago
|
||
(In reply to Peter Van der Beken [:peterv] from comment #10)
This chunk in ipc/ipdl/ipdl/lower.py makes us output a definition for methods that are marked as pure:
...
Note that pure was not set to 0 before, but methodspec is always set to MethodSpec.NONE now. visitMethodDefn checks for MethodSpec.PURE to decide whether to output a definition.Was that change in behaviour intended?
Is there a case where _splitMethodDefn() used to produce md.decl.pure == 1 even when md.decl.virtual and md.decl.override were 0?
I haven't looked at this code in a long while, but cgen.py's visitMethodDefn() returns early if md.decl.methodspec == MethodSpec.PURE, so we should not be emitting a pure method definition in C++. Can you share an example of the emitted C++ code?
Comment 12•5 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #11)
Is there a case where _splitMethodDefn() used to produce md.decl.pure == 1 even when md.decl.virtual and md.decl.override were 0?
For any case of md.decl.pure == 1, given that md.decl.virtual and md.decl.override were always explicitly set to 0 here and md.decl.pure wasn't touched.
I haven't looked at this code in a long while, but cgen.py's visitMethodDefn() returns early if md.decl.methodspec == MethodSpec.PURE, so we should not be emitting a pure method definition in C++.
As I pointed out, we now always set md.decl.methodspec to MethodSpec.NONE, never to MethodSpec.PURE.
Can you share an example of the emitted C++ code?
virtual void
ActorDestroy(ActorDestroyReason aWhy) = 0;
https://searchfox.org/mozilla-central/source/__GENERATED__/ipc/ipdl/PMediaParent.cpp#18
auto PMediaParent::ActorDestroy(ActorDestroyReason aWhy) -> void
{
}
Assignee | ||
Comment 13•5 years ago
|
||
(In reply to Peter Van der Beken [:peterv] from comment #12)
(In reply to Chris Peterson [:cpeterson] from comment #11)
Is there a case where _splitMethodDefn() used to produce md.decl.pure == 1 even when md.decl.virtual and md.decl.override were 0?
For any case of md.decl.pure == 1, given that md.decl.virtual and md.decl.override were always explicitly set to 0 here and md.decl.pure wasn't touched.
I see. I'll take a look.
auto PMediaParent::ActorDestroy(ActorDestroyReason aWhy) -> void { }
Defining pure virtual member function is valid C++, but just a waste of space for our use.
Description
•