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

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P3)

Description

[Chris Peterson [:cpeterson]]

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 1

[Chris Peterson [:cpeterson]]

Attachment: Bug 1428984 - Part 1: Consolidate virtual, pure, override, and static MethodDecl types into an MethodSpec enum.

pure or override implies virtual. static is mutually exclusive with virtual (and pure and override). Combining these types into a "method specifier" enum simplifies the code and prevents bogus states like virtual=0 and pure=1.

Review commit: https://reviewboard.mozilla.org/r/212812/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212812/

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: Bug 1428984 - Part 2: Remove unused never_inline flag.

never_inline is never set so cgen never emits MOZ_NEVER_INLINE.

Review commit: https://reviewboard.mozilla.org/r/212814/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212814/

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: Bug 1428984 - Part 3: Remove unused inline flag.

inline is never set so cgen never emits inline.

Review commit: https://reviewboard.mozilla.org/r/212816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212816/

Comment 4

[Chris Peterson [:cpeterson]]

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

[Nathan Froyd [:froydnj]]

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

[Nathan Froyd [:froydnj]]

Comment on attachment 8942561 [details]
Bug 1428984 - Part 2: Remove unused never_inline flag.

https://reviewboard.mozilla.org/r/212814/#review218886

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8942562 [details]
Bug 1428984 - Part 3: Remove unused inline flag.

https://reviewboard.mozilla.org/r/212816/#review218888

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

[Cosmin Sabou [:CosminS]]

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

[Peter Van der Beken [:peterv]]

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?

Comment 11

[Chris Peterson [: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

Comment 12

[Peter Van der Beken [:peterv]]

(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
{
}

Comment 13

[Chris Peterson [: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.