525181 - IPDL: Implement "soft" dynamic checking of protocol state in generated C++

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Fix up the stub C++ implementation of protocol states in actor classes. Add a special NULL state (that only transitions to -->Null and -->Dead) for stateless protocols

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Implement protocol state machines in C++

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Turn on "soft" state checking, NS_WARNING()ing on bad transitions

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Fix IPDL tests that violate their state machines and remove FIXME hacks around the lack of actor->state() in C++. To be qfolded with the previous patch

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

jduell, quick warning --- these patches by themselves probably won't be enough to implement state checking for necko (we're still a decent ways away from the whole enchilada).  Bug 533002 ought to be enough, though, so you might want to block on that.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Review ping.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Review ping.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Damn! How did I forget this again?!

Sorry, I'll get it tomorrow.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 450820 [details] [diff] [review]
Fix up the stub C++ implementation of protocol states in actor classes. Add a special NULL state (that only transitions to -->Null and -->Dead) for stateless protocols

>+        stmts.append(StmtExpr(ExprAssn(_actorState(actorvar),
>+                                       _startState(actorproto))))
>+
>+        return stmts

I don't quite get why you changed to using append() rather than adding to the list, is that intentional?

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 450821 [details] [diff] [review]
part 2: Implement protocol state machines in C++

>+    Trigger(Action _action, int32 _msg) :

Bah, leading underscores?

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 450822 [details] [diff] [review]
part 3: Turn on "soft" state checking, NS_WARNING()ing on bad transitions

>+        # FIXME: make this a FatalError()
>+        ifbad.addifstmt(_printWarningMessage('bad state transition!'))

You could make a _handleBadTransition function maybe? Then switching behavior down the road might be more intuitive.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #9)
> Comment on attachment 450820 [details] [diff] [review]
> Fix up the stub C++ implementation of protocol states in actor classes. Add a
> special NULL state (that only transitions to -->Null and -->Dead) for stateless
> protocols
> 
> >+        stmts.append(StmtExpr(ExprAssn(_actorState(actorvar),
> >+                                       _startState(actorproto))))
> >+
> >+        return stmts
> 
> I don't quite get why you changed to using append() rather than adding to the
> list, is that intentional?

Hm, looks like brain damage.  I think that code used to be in a |if hasstate:| block or similar and that the condition went away.  Thanks for the catch.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #10)
> Comment on attachment 450821 [details] [diff] [review]
> part 2: Implement protocol state machines in C++
> 
> >+    Trigger(Action _action, int32 _msg) :
> 
> Bah, leading underscores?

Alright alright, I'll use mFoo fields ;). *holds nose*

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #11)
> Comment on attachment 450822 [details] [diff] [review]
> part 3: Turn on "soft" state checking, NS_WARNING()ing on bad transitions
> 
> >+        # FIXME: make this a FatalError()
> >+        ifbad.addifstmt(_printWarningMessage('bad state transition!'))
> 
> You could make a _handleBadTransition function maybe? Then switching behavior
> down the road might be more intuitive.

Good idea, done.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/8bcc9c92b9c6
http://hg.mozilla.org/mozilla-central/rev/dc0f5b0bae97
http://hg.mozilla.org/mozilla-central/rev/2c920c17f9a3