Closed Bug 1428984 Opened 2 years ago Closed 2 years ago

Make override=1 and pure=1 imply virtual=1 to simplify MethodDecl

Categories

(Core :: IPC, enhancement, P3)

Unspecified
All
enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

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.
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 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.
Attachment #8942560 - Flags: review?(nfroyd) → review+
Comment on attachment 8942561 [details]
Bug 1428984 - Part 2: Remove unused never_inline flag.

https://reviewboard.mozilla.org/r/212814/#review218886
Attachment #8942561 - Flags: review?(nfroyd) → review+
Comment on attachment 8942562 [details]
Bug 1428984 - Part 3: Remove unused inline flag.

https://reviewboard.mozilla.org/r/212816/#review218888
Attachment #8942562 - Flags: review?(nfroyd) → review+
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

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?

Flags: needinfo?(cpeterson)

(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?

https://searchfox.org/mozilla-central/rev/38035ee92463c9e9fbca729ac7e66476ac7eb27a/ipc/ipdl/ipdl/cxx/cgen.py#258-260

Flags: needinfo?(cpeterson)

(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?

https://searchfox.org/mozilla-central/source/__GENERATED__/ipc/ipdl/_ipdlheaders/mozilla/media/PMediaParent.h#86

    virtual void
    ActorDestroy(ActorDestroyReason aWhy) = 0;

https://searchfox.org/mozilla-central/source/__GENERATED__/ipc/ipdl/PMediaParent.cpp#18

auto PMediaParent::ActorDestroy(ActorDestroyReason aWhy) -> void
{
}
Flags: needinfo?(cpeterson)

(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.

Flags: needinfo?(cpeterson)
Depends on: 1528452
You need to log in before you can comment on or make changes to this bug.