664339 - Remove _IF/_IFDEF/_IFNDEF processing code from exactgc.as and add failure conditions if they are encountered in future

Status: RESOLVED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P3)

Description

[Ruchi Lohani]

_IF/IFDEF/IFNDEF etc are no longer needed in the GC field annotations. They are handled by the exactgc.as. The script currently has code to process these. That code should be removed to clean up the script. Also, the script need to be modified to fail if it encounters any new _IF/_IFNDEF etc in annotations. 

The changes in bug #660013 are dependent on the fact that the code has no _IF field annotations. 

WE# 2891528 for removal of those in the player codebase. 
VM annotations were removed as part of bug#647184.

Comment 1

[Ruchi Lohani]

Attachment: Patch

Comment 2

[Lars T Hansen]

Comment on attachment 539382 [details] [diff] [review]
Patch

I'm going to SR+ this, but the better check for stray introductions of _IF etc would be to first test for indexOf("_IF") >= 0 (> 0 is technically correct but needlessly confusing, since for indexOf it's either -1 for failure or >= 0 for success), /then/ test via a regex whether it's specificially one of the patterns we've removed support for.  That way eg ETH_IF0 will not trigger the failure, which it should not.

Comment 3

[Ruchi Lohani]

(In reply to comment #2)
> Comment on attachment 539382 [details] [diff] [review] [review]
> Patch
> 
> I'm going to SR+ this, but the better check for stray introductions of _IF
> etc would be to first test for indexOf("_IF") >= 0 (> 0 is technically
> correct but needlessly confusing, since for indexOf it's either -1 for
> failure or >= 0 for success), /then/ test via a regex whether it's
> specificially one of the patterns we've removed support for.  That way eg
> ETH_IF0 will not trigger the failure, which it should not.

The check is only if (gcIndex >= 0) is true. 

While I agree that we should probably have specific checks for the deprecated annotations, I am also thinking if we should just update the docs to remove that _IFDEF stuff for fields and not have this check at all? Anyways, I am going to upload an updated patch with regex test. 

I deliberately did not mention anything about the _IF annotations in my GC-Dev kitchen preso so that those who don't know anything about annotations, should know nothing about these _IF cases :)

Comment 4

[Ruchi Lohani]

Attachment: Patch v1

Lars, is it ok to skip review for this. Asking only for SR at this point. Let me know if its not appropriate.

Comment 5

[Lars T Hansen]

Comment on attachment 539910 [details] [diff] [review]
Patch v1

Ship it.  And please also fix the docs to remove mention of the now-deprecated annotations.

Comment 6

[Ruchi Lohani]

Attachment: Doc changes

Removed conditional compilation tags for data members section.

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 541494 [details] [diff] [review]
Doc changes

(rubberstamp)

Comment 8

[Tamarin Bot]

changeset: 6424:6f57c798bd21
user:      Ruchi Lohani<[e-mail]>
summary:   Bug 664339 - Remove _IF/_IFDEF/_IFNDEF processing code from exactgc.as and add failure conditions (p=rulohani, r=treilly, sr=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/6f57c798bd21

Comment 9

[Tamarin Bot]

changeset: 6425:c5d8ba16d5ac
user:      Ruchi Lohani<[e-mail]>
summary:   Bug 664339 - Remove conditional compilation section from exactgc documentation(r=fklockii)

http://hg.mozilla.org/tamarin-redux/rev/c5d8ba16d5ac