668313 - exactgc.as parses the #ifdef condition incorrectly in case there are spaces before the condition

Status: RESOLVED FIXED

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

Description

[Ruchi Lohani]

exactgc.as will not parse this correctly:

class SomeClass {

private:
      #ifdef DEBUGGER
      GCMember<SomeOtherClass> randomMember;
      #endif

};

The conditional declaration here will appear something like this in the tracer .hh file:

#if defined(UGGER)   
    gc->TraceLocation(&randomMember);
#endif
[NOTE: DEBUGGER turned into UGGER or something else, depending upon the number of spaced before #ifdef]

I think this is what was causing AIR smoke failures after all the _IFDEF annotations removal.

Comment 1

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

My first instinct when I saw this bug was "its not legal to have whitespace before the octothorpe!"; then I realized I had no basis for asserting that.

It looks like my "knowledge" is based on pre-ANSI C limitations.

I am assuming Ruchi's plan is to extend exactgc.as to handle whitespace before the preprocessor directives.  (Which I'd happily go along with.)

See also, http://stackoverflow.com/questions/789073/indenting-defines

Comment 2

[Ruchi Lohani]

There are places in player where there are whitespaces before the '#' in #ifdef. So yes I would like to extend exactgc.as to handle whitespace. In the very short term I am just going to remove those whitespaces if that's what allows me to land the removal of _IFDEF etc annotations from player.

Comment 3

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

I think I have a fix.  Will be posting patch soon.

Comment 4

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

Attachment: fix condition extraction

From reading the comments in the area of the patch, it looks like the code was originally developed with a regexp that did not include ^\s*  before the octothorpe, and that's why there was the addition of cppIndex to calculate the offset to the start of the condition string.

But now we are including those leading spaces, and so the cppIndex should be left off.

Comment 5

[Ruchi Lohani]

Comment on attachment 543144 [details] [diff] [review]
fix condition extraction

Simple enough. I had thought of a different but probably more complicating way. Make the regex as /^\s*(#\s*(ifdef|ifndef|if|elif|else|endif|define))/ and then use result[1].length instead of result[0].length and result[2] for 'directive'.

Comment 6

[Tamarin Bot]

changeset: 6441:32102336f3c4
user:      Felix S Klock II <[e-mail]>
summary:   Bug 668313: exactgc.as fix condition name extraction (r=rulohani,sr=lhansen).

http://hg.mozilla.org/tamarin-redux/rev/32102336f3c4

Comment 7

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

(In reply to comment #5)
> Comment on attachment 543144 [details] [diff] [review] [review]
> fix condition extraction
> 
> Simple enough. I had thought of a different but probably more complicating
> way. Make the regex as /^\s*(#\s*(ifdef|ifndef|if|elif|else|endif|define))/
> and then use result[1].length instead of result[0].length and result[2] for
> 'directive'.

Nested captured groups, yum.  :)

I went with my version, I'm a fan of fixes that work solely by deleting code.

Comment 8

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

(it would be good to figure out a way to make selftests for things like this, if we aren't already, by the way.)