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

RESOLVED FIXED

Status

Tamarin
Garbage Collection (mmGC)
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Ruchi Lohani, Unassigned)

Tracking

(Blocks: 1 bug)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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.
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
(Reporter)

Comment 2

7 years ago
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.
I think I have a fix.  Will be posting patch soon.
Created attachment 543144 [details] [diff] [review]
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.
Attachment #543144 - Flags: superreview?(lhansen)
Attachment #543144 - Flags: review?(rulohani)
(Reporter)

Comment 5

7 years ago
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'.
Attachment #543144 - Flags: review?(rulohani) → review+

Updated

7 years ago
Attachment #543144 - Flags: superreview?(lhansen) → superreview+

Comment 6

7 years ago
changeset: 6441:32102336f3c4
user:      Felix S Klock II <fklockii@adobe.com>
summary:   Bug 668313: exactgc.as fix condition name extraction (r=rulohani,sr=lhansen).

http://hg.mozilla.org/tamarin-redux/rev/32102336f3c4
(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.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(it would be good to figure out a way to make selftests for things like this, if we aren't already, by the way.)
You need to log in before you can comment on or make changes to this bug.