Simplify SourceProvider's subclasses
Created attachment 141591 [details] Patch
Comment on attachment 141591 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141591&action=review This is OK as-is, but we might be able to do a bit better. > Source/JavaScriptCore/parser/SourceProvider.h:61 > + const StringImpl* data() const > + { > + return stringImpl(); > + } Why not leave this as a pure virtual function and call it instead of adding a new stringImpl() function? Or maybe this could all be done with the source() function? I am unclear on why there need to be three different functions that all return the source string. Also, all the callers seem to be calling impl() so maybe this should return a const UString& or const String&. That would be one way of saving a bit of reference count churn in the getRange function. I guess the source() function already works that way. > Source/JavaScriptCore/parser/SourceProvider.h:65 > + int length = end - start; Why a local variable for this? Doesn’t seem better than just saying end - start in the function call. > Source/JavaScriptCore/parser/SourceProvider.h:66 > + return UString(stringImpl()).substringSharingImpl(start, length); Would be nice to add a version of substringSharingImpl that works directly on a StringImpl* so we don’t have to churn the reference count an extra time each time this function is called. > Source/JavaScriptCore/parser/SourceProvider.h:72 > + StringImpl* impl = stringImpl(); > + return impl ? impl->length() : 0; There must be a better name than “impl” for this. Maybe “data” or “string”. Words are better than word fragments.
The method source() is actually WebCore specific. Generalizing it to JSC is a good idea, I will look into that.
With the UString change, this problem has radically changed. I close this bug as it is now superseded by https://bugs.webkit.org/show_bug.cgi?id=95635