| Summary: | Build fix for YarrParser.h | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Balazs Kelemen <kbalazs> | ||||
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | abecsi, barraclough, christoph, msaboff, ossy, pvarga | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | PC | ||||||
| OS: | Linux | ||||||
| Attachments: |
|
||||||
|
Description
Balazs Kelemen
2011-04-05 02:08:13 PDT
Created attachment 88191 [details]
Patch
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 *** 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; (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? *** Bug 55576 has been marked as a duplicate of this bug. *** (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. 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.
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.
(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! Comment on attachment 88191 [details] Patch Clearing flags on attachment: 88191 Committed r82988: <http://trac.webkit.org/changeset/82988> All reviewed patches have been landed. Closing bug. |