Summary: | Hopefully helpful comments for some String methods | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Victor Costan <costan> | ||||
Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | UNCONFIRMED --- | ||||||
Severity: | Normal | CC: | benjamin, cmarcelo, msaboff, ojan.autocc, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Victor Costan
2013-04-08 22:07:13 PDT
Created attachment 197003 [details]
Patch
Comment on attachment 197003 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197003&action=review > Source/WTF/wtf/text/WTFString.h:194 > + // Make sure the String is8Bit() before calling this. > const LChar* characters8() const > { > if (!m_impl) Why doesn't the ASSERT convey this meaning? Is there a way to improve the ASSERT so that it conveys this meaning? > Source/WTF/wtf/text/WTFString.h:200 > + // Make sure the String !is8Bit() before calling this. Same question. > Source/WTF/wtf/text/WTFString.h:223 > + // Unlike most String methods, this crashes if the String is null. > + // Use isNull() or isEmpty() to detect this special case. > + bool is8Bit() const > + { > + ASSERT(m_impl); > + return m_impl->is8Bit(); > + } I wonder if we should just fix this function instead of commenting around it. Thank you for the quick feedback, Geoffrey! When I read the code, I understood what it takes to make it crash. I didn't immediately understand that the recommended way of working on a String's characters is to have separate branches conditioned on is8Bit(). I wrote my conclusion as the comment for characters(). Should I copy it / point to it in the comments for characters{8, 16}() ? Would it be better to change is8Bit() to return true for null strings? Thank you! Comment on attachment 197003 [details]
Patch
I am okay with adding comments in wtf. I think it would be great.
Contrarily to WebCore, WTF does not change as fast but it is used by every code out there.
But I think we need proper usage documentation instead of comments here and there.
I should add, in my opinion characters8() and characters16() should not check for m_impl. I just never went around looking for what it would take. Benjamin: proper usage documentation would be awesome! At the same time, would you agree that this is a (tiny) step in the right direction? (In reply to comment #6) > Benjamin: proper usage documentation would be awesome! At the same time, would you agree that this is a (tiny) step in the right direction? No, not really. Take this: // Make sure the String is8Bit() before calling this. const LChar* characters8() const This is not strictly true. If you juste created a string with ASCIILiteral (or another 8bits constructor), you can call this method safely. Or this: // This is expensive for 8-bit strings. // // Write two branches of the code using characters8() and characters16(), // and use is8Bit() to branch. If the String can be null, use isNull() or // isEmpty() to handle that special case. This is explaining a particular pattern. It is also not strictly true that this method is expensive. If there is already a 16bits shadow, this is still pretty fast. Instead, if the reader knew what a WTF::String is, you should just be able to write: // Return a 16 bits representation of the string. If the string is 8 bits, the method // will upconvert the caracters. Comments should provide comprehension for the concepts. The information about details is already in the code, it does not need to be repeated. |