Bug 98105 – [CMAKE] Add GLOBAL_FASTMALLOC_NEW in cmakeconfig.h.cmake

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Jinwoo Song]

GLOBAL_FASTMALLOC_NEW feature is not included in cmakeconfig.h.cmake.

[Jinwoo Song]

Created attachment 167031 [details]
Patch

[Ryuan Choi]

Comment on attachment 167031 [details]
Patch

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

> ChangeLog:3
> +        [CMAKE] Add GLOBAL_FASTMALLOC_NEW in cmakeconfig.h.cmake

Other ports looks define it in Platform.h.

Should we need to enable/disable it in compile time?

[Zoltan Horvath]

Hi there,

It's enabled by default in Platform.h. I don't see how lead it ambiguity. Furthermore, this macro _should not_ be considered as a feature.

(In reply to comment #2)

> (From update of attachment 167031 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167031&action=review
> 
> > ChangeLog:3
> > +        [CMAKE] Add GLOBAL_FASTMALLOC_NEW in cmakeconfig.h.cmake
> 
> Other ports looks define it in Platform.h.
> 
> Should we need to enable/disable it in compile time?

You shouldn't change this macro until you don't have problems with customizing global memory operators on your platform.

I don't see any benefit from this change, so I would r- this patch, but I'm not a reviewer yet.

[Jinwoo Song]

(In reply to comment #3)
> Hi there,
> 
> It's enabled by default in Platform.h. I don't see how lead it ambiguity. Furthermore, this macro _should not_ be considered as a feature.
> 
> (In reply to comment #2)
> 
> > (From update of attachment 167031 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=167031&action=review
> > 
> > > ChangeLog:3
> > > +        [CMAKE] Add GLOBAL_FASTMALLOC_NEW in cmakeconfig.h.cmake
> > 
> > Other ports looks define it in Platform.h.
> > 
> > Should we need to enable/disable it in compile time?
> 
> You shouldn't change this macro until you don't have problems with customizing global memory operators on your platform.
> 
> I don't see any benefit from this change, so I would r- this patch, but I'm not a reviewer yet.

Okay, I got your point and I'll not change it. Thanks for good feedback. :)