I've been going through the WTF::String class for a patch I'm putting together, and I would like to add some comments that will make it easier for the next guy who needs to figure it out. I will attach a patch for this bug.
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.