Commit c24293a1 authored by barraclough@apple.com's avatar barraclough@apple.com
Browse files

Bug 36075 - Clean up screwyness re static string impls & Identifiers.

Reviewed by Oliver Hunt.

JavaScriptCore: 

* API/JSClassRef.cpp:
(OpaqueJSClass::~OpaqueJSClass): Classname may be null/empty, and these are an identifer.  This is okay, since the null/empty strings are shared across all threads.
* JavaScriptCore.exp:
* runtime/Identifier.cpp:
(JSC::Identifier::add): No need to explicitly hash null reps, this is done in the ststic UStringImpl constructor.
(JSC::Identifier::addSlowCase): UStringImpl::empty() handled & checkCurrentIdentifierTable now called in the header.
(JSC::Identifier::checkCurrentIdentifierTable): Replaces checkSameIdentifierTable (this no longer checked the rep since the identifierTable pointer was removed from UString::Rep long ago).
* runtime/Identifier.h:
(JSC::Identifier::add): Replace call to checkSameIdentifierTable with call to checkCurrentIdentifierTable at head of function.
* runtime/UStringImpl.cpp:
(JSC::UStringImpl::~UStringImpl): Remove call to checkConsistency - this function no longer checks anything interesting.
* runtime/UStringImpl.h:
(JSC::UStringOrRopeImpl::UStringOrRopeImpl): Set s_refCountFlagIsIdentifier in static constructor.
(JSC::UStringImpl::UStringImpl): remove calls to checkConsistency (see above), add new ASSERT to substring constructor.
(JSC::UStringImpl::setHash): ASSERT not static (static strings set the hash in their constructor, should not reach this code path).
(JSC::UStringImpl::create): Add missing ASSERT.
(JSC::UStringImpl::setIsIdentifier): ASSERT !isStatic() (static strings hash set in constructor).

WebCore: 

* platform/text/StringImpl.cpp:
(WebCore::StringImpl::~StringImpl): Add ASSERT
(WebCore::StringImpl::sharedBuffer): Add ASSERT
* platform/text/StringImpl.h:
(WebCore::StringImpl::setHash): Add ASSERT
(WebCore::StringImpl::isStatic): added.



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@55943 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent c167ac58
......@@ -111,7 +111,8 @@ OpaqueJSClass::OpaqueJSClass(const JSClassDefinition* definition, OpaqueJSClass*
OpaqueJSClass::~OpaqueJSClass()
{
ASSERT(!m_className.rep()->isIdentifier());
// The empty string is shared across threads & is an identifier, in all other cases we should have done a deep copy in className(), below.
ASSERT(!m_className.size() || !m_className.rep()->isIdentifier());
if (m_staticValues) {
OpaqueJSClassStaticValuesTable::const_iterator end = m_staticValues->end();
......
2010-03-11 Gavin Barraclough <barraclough@apple.com>
Reviewed by Oliver Hunt.
Bug 36075 - Clean up screwyness re static string impls & Identifiers.
* API/JSClassRef.cpp:
(OpaqueJSClass::~OpaqueJSClass): Classname may be null/empty, and these are an identifer. This is okay, since the null/empty strings are shared across all threads.
* JavaScriptCore.exp:
* runtime/Identifier.cpp:
(JSC::Identifier::add): No need to explicitly hash null reps, this is done in the ststic UStringImpl constructor.
(JSC::Identifier::addSlowCase): UStringImpl::empty() handled & checkCurrentIdentifierTable now called in the header.
(JSC::Identifier::checkCurrentIdentifierTable): Replaces checkSameIdentifierTable (this no longer checked the rep since the identifierTable pointer was removed from UString::Rep long ago).
* runtime/Identifier.h:
(JSC::Identifier::add): Replace call to checkSameIdentifierTable with call to checkCurrentIdentifierTable at head of function.
* runtime/UStringImpl.cpp:
(JSC::UStringImpl::~UStringImpl): Remove call to checkConsistency - this function no longer checks anything interesting.
* runtime/UStringImpl.h:
(JSC::UStringOrRopeImpl::UStringOrRopeImpl): Set s_refCountFlagIsIdentifier in static constructor.
(JSC::UStringImpl::UStringImpl): remove calls to checkConsistency (see above), add new ASSERT to substring constructor.
(JSC::UStringImpl::setHash): ASSERT not static (static strings set the hash in their constructor, should not reach this code path).
(JSC::UStringImpl::create): Add missing ASSERT.
(JSC::UStringImpl::setIsIdentifier): ASSERT !isStatic() (static strings hash set in constructor).
2010-03-12 Peter Varga <pvarga@inf.u-szeged.hu>
 
Reviewed by David Levin.
......
......@@ -92,8 +92,8 @@ __Z15jsRegExpExecutePK8JSRegExpPKtiiPii
__ZN14OpaqueJSString6createERKN3JSC7UStringE
__ZN3JSC10Identifier11addSlowCaseEPNS_12JSGlobalDataEPNS_11UStringImplE
__ZN3JSC10Identifier11addSlowCaseEPNS_9ExecStateEPNS_11UStringImplE
__ZN3JSC10Identifier24checkSameIdentifierTableEPNS_12JSGlobalDataEPNS_11UStringImplE
__ZN3JSC10Identifier24checkSameIdentifierTableEPNS_9ExecStateEPNS_11UStringImplE
__ZN3JSC10Identifier27checkCurrentIdentifierTableEPNS_12JSGlobalDataE
__ZN3JSC10Identifier27checkCurrentIdentifierTableEPNS_9ExecStateE
__ZN3JSC10Identifier3addEPNS_9ExecStateEPKc
__ZN3JSC10Identifier4fromEPNS_9ExecStateEi
__ZN3JSC10Identifier4fromEPNS_9ExecStateEj
......
......@@ -124,11 +124,8 @@ struct CStringTranslator {
PassRefPtr<UString::Rep> Identifier::add(JSGlobalData* globalData, const char* c)
{
if (!c) {
UString::Rep* rep = UString::null().rep();
rep->hash();
return rep;
}
if (!c)
return UString::null().rep();
if (!c[0])
return UString::Rep::empty();
if (!c[1])
......@@ -209,19 +206,18 @@ PassRefPtr<UString::Rep> Identifier::add(ExecState* exec, const UChar* s, int le
PassRefPtr<UString::Rep> Identifier::addSlowCase(JSGlobalData* globalData, UString::Rep* r)
{
ASSERT(!r->isIdentifier());
// The empty & null strings are static singletons, and static strings are handled
// in ::add() in the header, so we should never get here with a zero length string.
ASSERT(r->length());
if (r->length() == 1) {
UChar c = r->characters()[0];
if (c <= 0xFF)
r = globalData->smallStrings.singleCharacterStringRep(c);
if (r->isIdentifier()) {
#ifndef NDEBUG
checkSameIdentifierTable(globalData, r);
#endif
if (r->isIdentifier())
return r;
}
}
if (!r->length())
return UString::Rep::empty();
return *globalData->identifierTable->add(r).first;
}
......@@ -252,25 +248,24 @@ Identifier Identifier::from(ExecState* exec, double value)
#ifndef NDEBUG
void Identifier::checkSameIdentifierTable(ExecState* exec, UString::Rep*)
void Identifier::checkCurrentIdentifierTable(JSGlobalData* globalData)
{
ASSERT_UNUSED(exec, exec->globalData().identifierTable == currentIdentifierTable());
// Check the identifier table accessible through the threadspecific matches the
// globalData's identifier table.
ASSERT_UNUSED(globalData, globalData->identifierTable == currentIdentifierTable());
}
void Identifier::checkSameIdentifierTable(JSGlobalData* globalData, UString::Rep*)
void Identifier::checkCurrentIdentifierTable(ExecState* exec)
{
ASSERT_UNUSED(globalData, globalData->identifierTable == currentIdentifierTable());
checkCurrentIdentifierTable(&exec->globalData());
}
#else
void Identifier::checkSameIdentifierTable(ExecState*, UString::Rep*)
{
}
void Identifier::checkSameIdentifierTable(JSGlobalData*, UString::Rep*)
{
}
// These only exists so that our exports are the same for debug and release builds.
// This would be an ASSERT_NOT_REACHED(), but we're in NDEBUG only code here!
void Identifier::checkCurrentIdentifierTable(JSGlobalData*) { CRASH(); }
void Identifier::checkCurrentIdentifierTable(ExecState*) { CRASH(); }
#endif
......
......@@ -93,30 +93,28 @@ namespace JSC {
static PassRefPtr<UString::Rep> add(ExecState* exec, UString::Rep* r)
{
if (r->isIdentifier()) {
#ifndef NDEBUG
checkSameIdentifierTable(exec, r);
checkCurrentIdentifierTable(exec);
#endif
if (r->isIdentifier())
return r;
}
return addSlowCase(exec, r);
}
static PassRefPtr<UString::Rep> add(JSGlobalData* globalData, UString::Rep* r)
{
if (r->isIdentifier()) {
#ifndef NDEBUG
checkSameIdentifierTable(globalData, r);
checkCurrentIdentifierTable(globalData);
#endif
if (r->isIdentifier())
return r;
}
return addSlowCase(globalData, r);
}
static PassRefPtr<UString::Rep> addSlowCase(ExecState*, UString::Rep* r);
static PassRefPtr<UString::Rep> addSlowCase(JSGlobalData*, UString::Rep* r);
static void checkSameIdentifierTable(ExecState*, UString::Rep*);
static void checkSameIdentifierTable(JSGlobalData*, UString::Rep*);
static void checkCurrentIdentifierTable(ExecState*);
static void checkCurrentIdentifierTable(JSGlobalData*);
};
inline bool operator==(const Identifier& a, const Identifier& b)
......
......@@ -41,7 +41,6 @@ static const unsigned minLengthToShare = 20;
UStringImpl::~UStringImpl()
{
ASSERT(!isStatic());
checkConsistency();
if (isIdentifier())
Identifier::remove(this);
......
......@@ -72,7 +72,7 @@ protected:
enum StaticStringConstructType { ConstructStaticString };
UStringOrRopeImpl(unsigned length, StaticStringConstructType)
: m_refCountAndFlags(s_refCountFlagStatic | BufferOwned)
: m_refCountAndFlags(s_refCountFlagStatic | s_refCountFlagIsIdentifier | BufferOwned)
, m_length(length)
{
ASSERT(!isRope());
......@@ -125,7 +125,6 @@ private:
, m_hash(0)
{
hash();
checkConsistency();
}
// Create a normal string with internal storage (BufferInternal)
......@@ -137,7 +136,6 @@ private:
{
ASSERT(m_data);
ASSERT(m_length);
checkConsistency();
}
// Create a UStringImpl adopting ownership of the provided buffer (BufferOwned)
......@@ -149,7 +147,6 @@ private:
{
ASSERT(m_data);
ASSERT(m_length);
checkConsistency();
}
// Used to create new strings that are a substring of an existing UStringImpl (BufferSubstring)
......@@ -161,7 +158,7 @@ private:
{
ASSERT(m_data);
ASSERT(m_length);
checkConsistency();
ASSERT(m_substringBuffer->bufferOwnership() != BufferSubstring);
}
// Used to construct new strings sharing an existing SharedUChar (BufferShared)
......@@ -173,12 +170,12 @@ private:
{
ASSERT(m_data);
ASSERT(m_length);
checkConsistency();
}
// For use only by Identifier's XXXTranslator helpers.
void setHash(unsigned hash)
{
ASSERT(!isStatic());
ASSERT(!m_hash);
ASSERT(hash == computeHash(m_data, m_length));
m_hash = hash;
......@@ -193,10 +190,12 @@ public:
static PassRefPtr<UStringImpl> create(PassRefPtr<SharedUChar>, const UChar*, unsigned length);
static PassRefPtr<UStringImpl> create(PassRefPtr<UStringImpl> rep, unsigned offset, unsigned length)
{
ASSERT(rep);
ASSERT(length <= rep->length());
if (!length)
return empty();
ASSERT(rep);
rep->checkConsistency();
UStringImpl* ownerRep = (rep->bufferOwnership() == BufferSubstring) ? rep->m_substringBuffer : rep.get();
return adoptRef(new UStringImpl(rep->m_data + offset, length, ownerRep));
}
......@@ -247,6 +246,7 @@ public:
bool isIdentifier() const { return m_refCountAndFlags & s_refCountFlagIsIdentifier; }
void setIsIdentifier(bool isIdentifier)
{
ASSERT(!isStatic());
if (isIdentifier)
m_refCountAndFlags |= s_refCountFlagIsIdentifier;
else
......@@ -272,14 +272,6 @@ public:
memcpy(destination, source, numCharacters * sizeof(UChar));
}
ALWAYS_INLINE void checkConsistency() const
{
// There is no recursion of substrings.
ASSERT((bufferOwnership() != BufferSubstring) || (m_substringBuffer->bufferOwnership() != BufferSubstring));
// Static strings cannot be put in identifier tables, because they are globally shared.
ASSERT(!isStatic() || !isIdentifier());
}
private:
// This number must be at least 2 to avoid sharing empty, null as well as 1 character strings from SmallStrings.
static const unsigned s_copyCharsInlineCutOff = 20;
......@@ -287,7 +279,6 @@ private:
BufferOwnership bufferOwnership() const { return static_cast<BufferOwnership>(m_refCountAndFlags & s_refCountMaskBufferOwnership); }
bool isStatic() const { return m_refCountAndFlags & s_refCountFlagStatic; }
// unshared data
const UChar* m_data;
union {
void* m_buffer;
......
2010-03-11 Gavin Barraclough <barraclough@apple.com>
Reviewed by Oliver Hunt.
Bug 36075 - Clean up screwyness re static string impls & Identifiers.
* platform/text/StringImpl.cpp:
(WebCore::StringImpl::~StringImpl): Add ASSERT
(WebCore::StringImpl::sharedBuffer): Add ASSERT
* platform/text/StringImpl.h:
(WebCore::StringImpl::setHash): Add ASSERT
(WebCore::StringImpl::isStatic): added.
2010-03-12 Enrica Casucci <enrica@apple.com>
 
Reviewed by Simon Fraser.
......
......@@ -48,6 +48,8 @@ static const unsigned minLengthToShare = 20;
StringImpl::~StringImpl()
{
ASSERT(!isStatic());
if (inTable())
AtomicString::remove(this);
......@@ -129,6 +131,8 @@ SharedUChar* StringImpl::sharedBuffer()
{
if (m_length < minLengthToShare)
return 0;
// All static strings are smaller that the minimim length to share.
ASSERT(!isStatic());
BufferOwnership ownership = bufferOwnership();
......
......@@ -127,6 +127,7 @@ private:
// For use only by AtomicString's XXXTranslator helpers.
void setHash(unsigned hash)
{
ASSERT(!isStatic());
ASSERT(!m_hash);
ASSERT(hash == computeHash(m_data, m_length));
m_hash = hash;
......@@ -252,6 +253,7 @@ private:
static PassRefPtr<StringImpl> createStrippingNullCharactersSlowCase(const UChar*, unsigned length);
BufferOwnership bufferOwnership() const { return static_cast<BufferOwnership>(m_refCountAndFlags & s_refCountMaskBufferOwnership); }
bool isStatic() const { return m_refCountAndFlags & s_refCountFlagStatic; }
static const unsigned s_refCountMask = 0xFFFFFFE0;
static const unsigned s_refCountIncrement = 0x20;
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment