[webkit-reviews] review granted: [Bug 207119] Enable offlineasm debug annotations for GCC : [Attachment 395174] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Angelos Oikonomopoulos
<angelos at igalia.com>'s request for review:
Bug 207119: Enable offlineasm debug annotations for GCC
https://bugs.webkit.org/show_bug.cgi?id=207119

Attachment 395174: Patch

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




--- Comment #38 from Darin Adler <darin at apple.com> ---
Comment on attachment 395174
  --> https://bugs.webkit.org/attachment.cgi?id=395174
Patch

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

Looks fine to me; please consider further refinement to the Compiler.h macro

> Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:549
> +#endif /* WTF_COMPILER_GCC */

Code style nitpick:

This isn’t following the coding style of the rest of this file and WebKit. We
aren’t commenting every #endif, particularly when a block of code is this
short. And we use "//" comments when we do. And we don’t comment with the
underlying macro name instead of the actual expression on the #if statement.

Better to not have an #if here at all (see comment in Compiler.h).

> Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:557
> +#endif /* WTF_COMPILER_GCC */

Ditto.

> Source/WTF/wtf/Compiler.h:388
> +#if !defined(MARKER_SYMBOL) && COMPILER(GCC)
> +#define MARKER_SYMBOL(name) \
> +    __attribute__((__no_reorder__)) void name(void) { __asm__(""); }
> +#endif

It’s more consistent with the Compiler.h compiler-independence strategy to
define MARKER_SYMBOL everywhere and have it be a no-op on compilers that don’t
need it. Might also need a more unambiguous name. The phrase "marker symbol"
doesn’t point straight at a compiler capability to me. Seems a bit obscure to
claim such a basic name. I might have chosen a longer name like
DEBUGGER_ANNOTATION_MARKER(name). If we want to emphasize that it’s
GCC-specific can even include GCC in the name.