[webkit-reviews] review granted: [Bug 171199] [WTF] Move JSC tools/StackTrace to WTF and unify stack trace dump code : [Attachment 307952] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 171199: [WTF] Move JSC tools/StackTrace to WTF and unify stack trace dump
code
https://bugs.webkit.org/show_bug.cgi?id=171199

Attachment 307952: Patch

https://bugs.webkit.org/attachment.cgi?id=307952&action=review




--- Comment #6 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 307952
  --> https://bugs.webkit.org/attachment.cgi?id=307952
Patch

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

LGTM but need to fix broken Windows build.

> Source/JavaScriptCore/ChangeLog:8
> +	   This patch adds an utility method to produce demangled names with
dladdr.

typo: /an utility/a utility/.

> Source/WTF/wtf/DefaultFree.h:33
> +struct DefaultFree {

I suggest renaming this to SystemFree to match our parlance elsewhere e.g. see
DebugHeap::DebugHeap()'s referral to "System Malloc"  in
bmalloc/heap/DebugHeap.cpp.

> Source/WTF/wtf/Platform.h:673
> +#endif

I suggest adding // PLATFORM(GTK) after the #endif.

> Source/WTF/wtf/Platform.h:675
> +#endif

I suggest adding // OS(DARWIN) || OS(LINUX) after the #endif.

> Source/WTF/wtf/StackTrace.h:61
> +	   DemangleEntry(const char* mangledName, const char* demangledName)
> +	       : m_mangledName(mangledName)
> +	       , m_demangledName(demangledName)
> +	   { }

I suggest making this constructor private and adding StackTrace as a friend of
DemangleEntry.	This way, you can control the life-cycle of DemangleEntry
tightly and ensure that demangledName is indeed allocated with the system
malloc to match how we free it.