Bug 150555 – Replace 0 and NULL with nullptr in WebCore/editing.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Hunseop Jeong]

Use the 'nullptr' instead of '0' and 'NULL' in WebCore/editing.

[Hunseop Jeong]

Created attachment 264042 [details]
Patch

[Darin Adler]

Comment on attachment 264042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264042&action=review

> Source/WebCore/editing/ApplyStyleCommand.cpp:133
> -    , m_isInlineElementToRemoveFunction(0)
> +    , m_isInlineElementToRemoveFunction(nullptr)

Better in a case like this to move the initialization to nullptr to the class definition and remove it from all of the constructors.

> Source/WebCore/editing/ApplyStyleCommand.cpp:563
> +    RefPtr<ContainerNode> startDummySpanAncestor = nullptr;
> +    RefPtr<ContainerNode> endDummySpanAncestor = nullptr;

No need to initialize RefPtr to null; it’s null even if you don’t specify anything. It’s only raw pointers that need nullptr.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:108
> +    , m_startBlock(nullptr)
> +    , m_endBlock(nullptr)
> +    , m_typingStyle(nullptr)
> +    , m_deleteIntoBlockquoteStyle(nullptr)

Better in a case like this to move the initialization to nullptr to the class definition and remove it from the constructor.

> Source/WebCore/editing/EditCommand.cpp:43
> +    , m_parent(nullptr)

Better in a case like this to move the initialization to nullptr to the class definition and remove it from all of the constructors.

> Source/WebCore/editing/EditingStyle.cpp:178
> +    , m_tagName(nullptr)

Better in a case like this to move the initialization to nullptr to the class definition and remove it from all of the constructors.

> Source/WebCore/editing/EditorCommand.cpp:1680
> +    : m_command(nullptr)

Better in a case like this to move the initialization to nullptr to the class definition and remove it from all of the constructors.

> Source/WebCore/editing/RenderedPosition.cpp:68
> +    : m_renderer(nullptr)
> +    , m_inlineBox(nullptr)

Better in a case like this to move the initialization to nullptr to the class definition and remove it from all of the constructors.

> Source/WebCore/editing/RenderedPosition.h:96
> +    : m_renderer(nullptr)
> +    , m_inlineBox(nullptr)

Better in a case like this to move the initialization to nullptr to the class definition and remove it from all of the constructors.

> Source/WebCore/editing/SmartReplaceCF.cpp:40
> +    static CFMutableCharacterSetRef preSmartSet = nullptr;
> +    static CFMutableCharacterSetRef postSmartSet = nullptr;

No need to initialize a global variable (static) to nullptr. That will happen even if it’s not specified. Better to just remove the initialization entirely.

> Source/WebCore/editing/SpellChecker.cpp:47
> +    : m_checker(nullptr)

Better in a case like this to move the initialization to nullptr to the class definition and remove it from the constructor.

> Source/WebCore/editing/VisibleUnits.cpp:140
> +CachedLogicallyOrderedLeafBoxes::CachedLogicallyOrderedLeafBoxes() : m_rootInlineBox(nullptr) { };

Better in a case like this to move the initialization to nullptr to the class definition and remove it from the constructor. Also, there’s an extra semicolon here. And no good reason to format this all on one long line instead of normal formatting.

> Source/WebCore/editing/ios/EditorIOS.mm:125
> +    const char *newValue = nil;

This use of nil is incorrect. You should only use nil for Objective-C object pointers, not plain C ones. It would be OK to use nullptr here.

> Source/WebCore/editing/ios/EditorIOS.mm:207
> +    const Font* font = nil;

This use of nil is incorrect. You should only use nil for Objective-C object pointers, not C++ object pointers. It would be OK to use nullptr here.

> Source/WebCore/editing/mac/EditorMac.mm:130
> +    const Font* font = nil;

This use of nil is incorrect. You should only use nil for Objective-C object pointers, not C++ object pointers. It would be OK to use nullptr here.

[Hunseop Jeong]

Created attachment 264125 [details]
Patch

[Hunseop Jeong]

(In reply to comment #2)
> Comment on attachment 264042 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264042&action=review
> 
> > Source/WebCore/editing/ApplyStyleCommand.cpp:133
> > -    , m_isInlineElementToRemoveFunction(0)
> > +    , m_isInlineElementToRemoveFunction(nullptr)
> 
> Better in a case like this to move the initialization to nullptr to the
> class definition and remove it from all of the constructors.
I moved the initialization to nullptr to the class definition and remove it from all of the constructors except the 'm_isInlineElementToRemoveFunction(isInlineElementToRemoveFunction)' in the last constructor.
> 
> > Source/WebCore/editing/ApplyStyleCommand.cpp:563
> > +    RefPtr<ContainerNode> startDummySpanAncestor = nullptr;
> > +    RefPtr<ContainerNode> endDummySpanAncestor = nullptr;
> 
> No need to initialize RefPtr to null; it’s null even if you don’t specify
> anything. It’s only raw pointers that need nullptr.
I see. Do I leave the RefPtr without initialization in this case?
> 
> > Source/WebCore/editing/DeleteSelectionCommand.cpp:108
> > +    , m_startBlock(nullptr)
> > +    , m_endBlock(nullptr)
> > +    , m_typingStyle(nullptr)
> > +    , m_deleteIntoBlockquoteStyle(nullptr)
> 
> Better in a case like this to move the initialization to nullptr to the
> class definition and remove it from the constructor.
I removed the initialization to nullptr from all of the contructors but I didn't move the initialization to class definition because it was RefPtr.
> 
> > Source/WebCore/editing/EditCommand.cpp:43
> > +    , m_parent(nullptr)
> 
> Better in a case like this to move the initialization to nullptr to the
> class definition and remove it from all of the constructors.
I moved the initialization to nullptr to the class definition and removed it from all of the contructors.
> 
> > Source/WebCore/editing/EditingStyle.cpp:178
> > +    , m_tagName(nullptr)
> 
> Better in a case like this to move the initialization to nullptr to the
> class definition and remove it from all of the constructors.
I moved the initialization to nullptr to the class definition and removed it from the contructor.
> 
> > Source/WebCore/editing/EditorCommand.cpp:1680
> > +    : m_command(nullptr)
> 
> Better in a case like this to move the initialization to nullptr to the
> class definition and remove it from all of the constructors.
I moved the initialization to nullptr to the class definition and removed it from the contructor.
> 
> > Source/WebCore/editing/RenderedPosition.cpp:68
> > +    : m_renderer(nullptr)
> > +    , m_inlineBox(nullptr)
> 
> Better in a case like this to move the initialization to nullptr to the
> class definition and remove it from all of the constructors.
I moved the initialization to nullptr to the class definition and removed it from all of the contructors.
> 
> > Source/WebCore/editing/RenderedPosition.h:96
> > +    : m_renderer(nullptr)
> > +    , m_inlineBox(nullptr)
> 
> Better in a case like this to move the initialization to nullptr to the
> class definition and remove it from all of the constructors.
I moved the initialization to nullptr to the class definition and removed it from all of the contructors.
> 
> > Source/WebCore/editing/SmartReplaceCF.cpp:40
> > +    static CFMutableCharacterSetRef preSmartSet = nullptr;
> > +    static CFMutableCharacterSetRef postSmartSet = nullptr;
> 
> No need to initialize a global variable (static) to nullptr. That will
> happen even if it’s not specified. Better to just remove the initialization
> entirely.
I see. I removed the initialization entirely.
> 
> > Source/WebCore/editing/SpellChecker.cpp:47
> > +    : m_checker(nullptr)
> 
> Better in a case like this to move the initialization to nullptr to the
> class definition and remove it from the constructor.
I moved the initialization to nullptr to the class definition and removed it from the contructor.
> 
> > Source/WebCore/editing/VisibleUnits.cpp:140
> > +CachedLogicallyOrderedLeafBoxes::CachedLogicallyOrderedLeafBoxes() : m_rootInlineBox(nullptr) { };
> 
> Better in a case like this to move the initialization to nullptr to the
> class definition and remove it from the constructor. Also, there’s an extra
> semicolon here. And no good reason to format this all on one long line
> instead of normal formatting.
I moved the initialization to nullptr to the class definition and removed it from the constructor. And changed the format to follow the normal formatting.

> 
> > Source/WebCore/editing/ios/EditorIOS.mm:125
> > +    const char *newValue = nil;
> 
> This use of nil is incorrect. You should only use nil for Objective-C object
> pointers, not plain C ones. It would be OK to use nullptr here.
Changed to nullptr.
> 
> > Source/WebCore/editing/ios/EditorIOS.mm:207
> > +    const Font* font = nil;
> 
> This use of nil is incorrect. You should only use nil for Objective-C object
> pointers, not C++ object pointers. It would be OK to use nullptr here.
Ditto.
> 
> > Source/WebCore/editing/mac/EditorMac.mm:130
> > +    const Font* font = nil;
> 
> This use of nil is incorrect. You should only use nil for Objective-C object
> pointers, not C++ object pointers. It would be OK to use nullptr here.
Ditto.

Darin, Thanks for the review.
I always learn many things from you.
Thanks.

[Darin Adler]

Comment on attachment 264125 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264125&action=review

> Source/WebCore/editing/mac/AlternativeTextUIController.mm:53
>      HashMapType::const_iterator itr = m_alternativesObjectMap.find(context);
>      if (itr == m_alternativesObjectMap.end())
> -        return NULL;
> +        return nil;
>      return itr->value.get();

I just realized that all this logic is doing the same thing get would do, since the default value is nil. We can just write:

    return m_alternativesObjectMap.get(context);

[Hunseop Jeong]

Created attachment 264198 [details]
Patch

[Hunseop Jeong]

(In reply to comment #5)
> Comment on attachment 264125 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264125&action=review
> 
> > Source/WebCore/editing/mac/AlternativeTextUIController.mm:53
> >      HashMapType::const_iterator itr = m_alternativesObjectMap.find(context);
> >      if (itr == m_alternativesObjectMap.end())
> > -        return NULL;
> > +        return nil;
> >      return itr->value.get();
> 
> I just realized that all this logic is doing the same thing get would do,
> since the default value is nil. We can just write:
> 
>     return m_alternativesObjectMap.get(context);

If we used the 'return m_alternativesObjectMap.get(context)', we got the build break because alternativesObjectMap value is RetainPtr.
Is it correct to use the 'm_alternativesObjectMap.get(context).get()'?

[Hunseop Jeong]

Created attachment 264201 [details]
Patch

[Darin Adler]

Comment on attachment 264201 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264201&action=review

> Source/WebCore/editing/mac/AlternativeTextUIController.mm:50
> +    return m_alternativesObjectMap.get(context).get();

Yes, this is correct.

For RefPtr, we have traits that make hash maps know how to return raw pointers instead of returning RetainPtr, but I guess we didn’t do that for RetainPtr. Someone can come back and fix that at some point.

[WebKit Commit Bot]

Comment on attachment 264201 [details]
Patch

Clearing flags on attachment: 264201

Committed r191671: <http://trac.webkit.org/changeset/191671>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.