Bug 57822 – Build fix for YarrParser.h

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Balazs Kelemen]

It does not compile for a long time in my local environment which is: gcc (SUSE Linux) 4.5.0 20100604 [gcc-4_5-branch revision 160292].

[Balazs Kelemen]

Created attachment 88191 [details]
Patch

[Csaba Osztrogonác]

I'm not sure which initialization value should we 
use, ask the Yarr experts, Cpt Stampho and/or Gavin.

*** This bug has been marked as a duplicate of bug 55576 ***

[Balazs Kelemen]

The other bug has not patch and I won't re-upload mine.
0 is seems like a natural value for UChar. Here is some grep output:
Source/JavaScriptCore/runtime/StringPrototype.cpp:865:    UChar ored = 0;
Source/JavaScriptCore/runtime/StringPrototype.cpp:905:    UChar ored = 0;
Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:81:    UChar u = 0;

[Csaba Osztrogonác]

(In reply to comment #3)
> The other bug has not patch and I won't re-upload mine.
Nobody asked you to upload your patch again ...

> 0 is seems like a natural value for UChar. Here is some grep output:
> Source/JavaScriptCore/runtime/StringPrototype.cpp:865:    UChar ored = 0;
> Source/JavaScriptCore/runtime/StringPrototype.cpp:905:    UChar ored = 0;
> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:81:    UChar u = 0;

It is absolutely useless information. We can't fix the bug without understanding the Yarr parser. I don't know if m_character is used
somewhere before initializing. But using a possibly wrong init value
is so wrong as using an uninitialized value.

Gavin, could you check if this initialization is correct or not?

[Csaba Osztrogonác]

*** Bug 55576 has been marked as a duplicate of this bug. ***

[Balazs Kelemen]

(In reply to comment #4)
> (In reply to comment #3)
> > The other bug has not patch and I won't re-upload mine.
> Nobody asked you to upload your patch again ...
> 
> > 0 is seems like a natural value for UChar. Here is some grep output:
> > Source/JavaScriptCore/runtime/StringPrototype.cpp:865:    UChar ored = 0;
> > Source/JavaScriptCore/runtime/StringPrototype.cpp:905:    UChar ored = 0;
> > Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:81:    UChar u = 0;
> 
> It is absolutely useless information. We can't fix the bug without understanding the Yarr parser. I don't know if m_character is used
> somewhere before initializing.

Don't you think that 0 is better than memory garbage? What else can be the initialization value for an integral type? I don't understand your concerns.

[Darin Adler]

Comment on attachment 88191 [details]
Patch

I think that we don’t want to initialize things just on general principles or because “initialized is better than uninitialized”. It’s best to set the initial value based on an understanding of how the value will be used.

[Darin Adler]

Comment on attachment 88191 [details]
Patch

If we do make this change, it will be just to make the compiler happy. When m_state is Empty it is illegal to look at the character value, so initializing it is only helpful to mitigate programming mistakes. If m_state is set to another state where m_character is used, then m_character will be set appropriately.

The compiler can’t analyze the data flow well enough to see the relationship so it thinks there is a potential problem here, but there is none.

[Balazs Kelemen]

(In reply to comment #8)
> (From update of attachment 88191 [details])
> If we do make this change, it will be just to make the compiler happy. When m_state is Empty it is illegal to look at the character value, so initializing it is only helpful to mitigate programming mistakes. If m_state is set to another state where m_character is used, then m_character will be set appropriately.
> 
> The compiler can’t analyze the data flow well enough to see the relationship so it thinks there is a potential problem here, but there is none.

Sure. The only motivation behind this patch is to make the compiler happy.
Thanks for the review!

[Balazs Kelemen]

Comment on attachment 88191 [details]
Patch

Clearing flags on attachment: 88191

Committed r82988: <http://trac.webkit.org/changeset/82988>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.