[webkit-reviews] review denied: [Bug 237329] [XCBuild] Emit a discovered dependency file from offlineasm : [Attachment 453560] Integrate offlineasm using build rules and emit a depfile r5

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has denied Elliott Williams
<emw at apple.com>'s request for review:
Bug 237329: [XCBuild] Emit a discovered dependency file from offlineasm
https://bugs.webkit.org/show_bug.cgi?id=237329

Attachment 453560: Integrate offlineasm using build rules and emit a depfile r5

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




--- Comment #8 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 453560
  --> https://bugs.webkit.org/attachment.cgi?id=453560
Integrate offlineasm using build rules and emit a depfile r5

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

> Source/JavaScriptCore/offlineasm/asm.rb:391
> +if FileTest.exist?(outputFlnm) and not $options[:depfile] or
FileTest.exist?($options[:depfile])

Can we put parens here that way it's trivial to remember if this is:
`FileTest.exist?(outputFlnm) and (not $options[:depfile] or
FileTest.exist?($options[:depfile]))` vs `(FileTest.exist?(outputFlnm) and not
$options[:depfile]) or FileTest.exist?($options[:depfile])`. I also think it's
wrong without the parens anyway (IIRC, this is the latter).

> Source/JavaScriptCore/offlineasm/generate_offset_extractor.rb:78
> +if FileTest.exist?(outputFlnm) and not $options[:depfile] or
FileTest.exist?($options[:depfile])

Ditto.

> Source/JavaScriptCore/offlineasm/generate_settings_extractor.rb:60
> +if FileTest.exist?(outputFlnm) and not $options[:depfile] or
FileTest.exist?($options[:depfile])

Ditto.