1335886 - Remove vestigial symtab property on nodes in IPDL type checker

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Andrew McCreight [:mccr8]]

The type checker adds "symtab" properties to nodes that it calls enterScope() on, which are a frame of the symbol table. However, this is only ever used to detect recursive loops. Getting rid of this makes it clearer how symbol tables are actually used (namely, there is only one ever live at any given time).

As an additional minor cleanup, the symtab property is used on GatherDecls but not CheckTypes, and is always initially null.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Bug 1335886, part 1 - Set symtab on GatherDecls instead of TcheckVisitor.

It is always initially set to None, and the other subclass of
TcheckVisitor (CheckTypes) never uses it.

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

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Bug 1335886, part 2 - Nodes never have existing symbol tables in enterScope.

This code gets deleted in the next patch, but the point here is that
we can still compile Firefox with this assertion in place.

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

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Bug 1335886, part 3 - Don't use symtab property to avoid infinite recursion.

Instead, just stick a "visited" property on them. The symtab property
on nodes isn't used for any other purpose.

The next patch will eliminate the unused args to enterScope and
exitScope.

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

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Bug 1335886, part 4 - Remove the unused node arg to {enter,exit}Scope.

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

Comment 5

[Andrew McCreight [:mccr8]]

My testing here was limited to making sure that the tree built after every change set. I'm pretty sure they also did not change the generated code.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8832623 [details]
Bug 1335886, part 1 - Set symtab on GatherDecls instead of TcheckVisitor.

https://reviewboard.mozilla.org/r/108804/#review110438

::: ipc/ipdl/ipdl/type.py:539
(Diff revision 1)
>      def declare(self, loc, type, shortname=None, fullname=None, progname=None):
>          d = Decl(loc)
>          d.type = type
>          d.progname = progname
>          d.shortname = shortname
>          d.fullname = fullname
>          self.symtab.declare(d)
>          return d

Can you move this method to GatherDecls since it's only used there?

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8832624 [details]
Bug 1335886, part 2 - Nodes never have existing symbol tables in enterScope.

https://reviewboard.mozilla.org/r/108806/#review110442

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8832625 [details]
Bug 1335886, part 3 - Don't use symtab property to avoid infinite recursion.

https://reviewboard.mozilla.org/r/108808/#review110444

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8832626 [details]
Bug 1335886, part 4 - Remove the unused node arg to {enter,exit}Scope.

https://reviewboard.mozilla.org/r/108810/#review110446

Comment 10

[Andrew McCreight [:mccr8]]

(In reply to Bill McCloskey (:billm) from comment #6)
> Can you move this method to GatherDecls since it's only used there?

Good point. I'll do that.

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 8832623 [details]
Bug 1335886, part 1 - Set symtab on GatherDecls instead of TcheckVisitor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108804/diff/1-2/

Comment 12

[Andrew McCreight [:mccr8]]

Comment on attachment 8832624 [details]
Bug 1335886, part 2 - Nodes never have existing symbol tables in enterScope.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108806/diff/1-2/

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 8832625 [details]
Bug 1335886, part 3 - Don't use symtab property to avoid infinite recursion.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108808/diff/1-2/

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 8832626 [details]
Bug 1335886, part 4 - Remove the unused node arg to {enter,exit}Scope.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108810/diff/1-2/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3dc3434f7a7
part 1 - Set symtab on GatherDecls instead of TcheckVisitor. r=billm
https://hg.mozilla.org/integration/autoland/rev/fb72e2cea50f
part 2 - Nodes never have existing symbol tables in enterScope. r=billm
https://hg.mozilla.org/integration/autoland/rev/3abddeffa872
part 3 - Don't use symtab property to avoid infinite recursion. r=billm
https://hg.mozilla.org/integration/autoland/rev/b66afe0be0f9
part 4 - Remove the unused node arg to {enter,exit}Scope. r=billm

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b3dc3434f7a7
https://hg.mozilla.org/mozilla-central/rev/fb72e2cea50f
https://hg.mozilla.org/mozilla-central/rev/3abddeffa872
https://hg.mozilla.org/mozilla-central/rev/b66afe0be0f9