Summary: | QualifiedName should use RefPtr<QualifiedNameImpl> internally. | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||
Component: | DOM | Assignee: | Andreas Kling <kling> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | commit-queue, esprehn+autocc, kangil.han, kling | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Andreas Kling
2014-05-10 22:39:33 PDT
Created attachment 231250 [details]
Patch
Attachment 231250 [details] did not pass style-queue:
ERROR: Source/WebCore/dom/QualifiedName.h:74: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
ERROR: Source/WebCore/dom/QualifiedName.h:81: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
Total errors found: 2 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 231250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231250&action=review These are the Windows build errors: 1>..\dom\QualifiedName.cpp(100): error C2734: 'WebCore::anyName' : const object must be initialized if not extern 1>..\dom\QualifiedName.cpp(100): warning C4211: nonstandard extension used : redefined extern to static 1>..\dom\QualifiedName.cpp(100): error C2512: 'WebCore::QualifiedName' : no appropriate default constructor available Looks good as long as you resolve that before landing. I think QNAME_DEFAULT_CONSTRUCTOR was doing some good on Windows. > Source/WebCore/dom/QualifiedName.cpp:81 > QualifiedNameComponents components = { p.impl(), l.impl(), n.isEmpty() ? nullAtom.impl() : n.impl() }; Isn’t nullAtom.impl() the same thing as nullptr? > Source/WebCore/dom/QualifiedName.cpp:83 > + m_impl = addResult.isNewEntry ? adoptRef(*addResult.iterator) : *addResult.iterator; Can we make an inline helper function to do this work? Then we can use construction instead of assignment for m_impl, and possibly even save a branch. I also think we could go nuts and use words for the three arguments instead of letters. > Source/WebCore/dom/QualifiedName.h:74 > + explicit QualifiedName(WTF::HashTableDeletedValueType) : m_impl(WTF::HashTableDeletedValue) { } Are the explicit WTF prefixes needed here? Fine to leave them in if they are indeed needed. > Source/WebCore/dom/QualifiedName.h:76 > + ~QualifiedName() { } I believe the best way to get this effect, a public empty destructor that gets inlined, is to omit explicit declaration or definition of the destructor entirely. > Source/WebCore/dom/QualifiedName.h:78 > QualifiedName() : m_impl(0) { } Maybe nullptr this while you are in the neighborhood. Committed r170816: <http://trac.webkit.org/changeset/170816> |