Bug 114240 – Hopefully helpful comments for some String methods

• Status: UNCONFIRMED
• Component: WebKit Misc.

[Victor Costan]

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.

[Victor Costan]

Created attachment 197003 [details]
Patch

[Geoffrey Garen]

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.

[Victor Costan]

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!

[Benjamin Poulain]

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.

[Benjamin Poulain]

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.

[Victor Costan]

Benjamin: proper usage documentation would be awesome! At the same time, would you agree that this is a (tiny) step in the right direction?

[Benjamin Poulain]

(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.