[webkit-reviews] review granted: [Bug 135269] iOS Simulator LayoutTestRelay : [Attachment 235576] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted David Farler
<dfarler at apple.com>'s request for review:
Bug 135269: iOS Simulator LayoutTestRelay
https://bugs.webkit.org/show_bug.cgi?id=135269

Attachment 235576: Patch
https://bugs.webkit.org/attachment.cgi?id=235576&action=review

------- Additional Comments from David Kilzer (:ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=235576&action=review


r=me, but please email webkit-dev first, and consider the feedback.

> Tools/LayoutTestRelay/LayoutTestRelay.xcodeproj/project.pbxproj:262
> +		2EB3B11C196F095F00ED4429 /* Debug */ = {
> +			isa = XCBuildConfiguration;
> +			buildSettings = {
> +				ALWAYS_SEARCH_USER_PATHS = NO;
> +				CLANG_CXX_LANGUAGE_STANDARD = "gnu++0x";
> +				CLANG_CXX_LIBRARY = "libc++";
> +				CLANG_ENABLE_MODULES = YES;
> +				CLANG_ENABLE_OBJC_ARC = YES;
> +				CLANG_WARN_BOOL_CONVERSION = YES;
> +				CLANG_WARN_CONSTANT_CONVERSION = YES;
> +				CLANG_WARN_DIRECT_OBJC_ISA_USAGE = YES_ERROR;
> +				CLANG_WARN_EMPTY_BODY = YES;
> +				CLANG_WARN_ENUM_CONVERSION = YES;
> +				CLANG_WARN_INT_CONVERSION = YES;
> +				CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR;
> +				CLANG_WARN_UNREACHABLE_CODE = YES;
> +				CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;
> +				COPY_PHASE_STRIP = NO;
> +				ENABLE_STRICT_OBJC_MSGSEND = YES;
> +				GCC_C_LANGUAGE_STANDARD = gnu99;
> +				GCC_DYNAMIC_NO_PIC = NO;
> +				GCC_OPTIMIZATION_LEVEL = 0;
> +				GCC_PREPROCESSOR_DEFINITIONS = (
> +					"DEBUG=1",
> +					"$(inherited)",
> +				);
> +				GCC_SYMBOLS_PRIVATE_EXTERN = NO;
> +				GCC_WARN_64_TO_32_BIT_CONVERSION = YES;
> +				GCC_WARN_ABOUT_RETURN_TYPE = YES_ERROR;
> +				GCC_WARN_UNDECLARED_SELECTOR = YES;
> +				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
> +				GCC_WARN_UNUSED_FUNCTION = YES;
> +				GCC_WARN_UNUSED_VARIABLE = YES;
> +				MACOSX_DEPLOYMENT_TARGET = 10.10;
> +				MTL_ENABLE_DEBUG_INFO = YES;
> +				ONLY_ACTIVE_ARCH = YES;
> +				SDKROOT = macosx;
> +				SWIFT_OPTIMIZATION_LEVEL = "-Onone";
> +			};
> +			name = Debug;
> +		};
> +		2EB3B11D196F095F00ED4429 /* Release */ = {
> +			isa = XCBuildConfiguration;
> +			buildSettings = {
> +				ALWAYS_SEARCH_USER_PATHS = NO;
> +				CLANG_CXX_LANGUAGE_STANDARD = "gnu++0x";
> +				CLANG_CXX_LIBRARY = "libc++";
> +				CLANG_ENABLE_MODULES = YES;
> +				CLANG_ENABLE_OBJC_ARC = YES;
> +				CLANG_WARN_BOOL_CONVERSION = YES;
> +				CLANG_WARN_CONSTANT_CONVERSION = YES;
> +				CLANG_WARN_DIRECT_OBJC_ISA_USAGE = YES_ERROR;
> +				CLANG_WARN_EMPTY_BODY = YES;
> +				CLANG_WARN_ENUM_CONVERSION = YES;
> +				CLANG_WARN_INT_CONVERSION = YES;
> +				CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR;
> +				CLANG_WARN_UNREACHABLE_CODE = YES;
> +				CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;
> +				COPY_PHASE_STRIP = YES;
> +				DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym";
> +				ENABLE_NS_ASSERTIONS = NO;
> +				ENABLE_STRICT_OBJC_MSGSEND = YES;
> +				GCC_C_LANGUAGE_STANDARD = gnu99;
> +				GCC_WARN_64_TO_32_BIT_CONVERSION = YES;
> +				GCC_WARN_ABOUT_RETURN_TYPE = YES_ERROR;
> +				GCC_WARN_UNDECLARED_SELECTOR = YES;
> +				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
> +				GCC_WARN_UNUSED_FUNCTION = YES;
> +				GCC_WARN_UNUSED_VARIABLE = YES;
> +				MACOSX_DEPLOYMENT_TARGET = 10.10;
> +				MTL_ENABLE_DEBUG_INFO = NO;
> +				SDKROOT = macosx;
> +			};
> +			name = Release;
> +		};

We should extract these settings out into *.xcconfig files in a Configuration
directory like all the other projects.

> Tools/LayoutTestRelay/LayoutTestRelay/Array.swift:26
> +// <rdar://problem/17768202>

We don't have to be so opaque here.  Add a title like:	Swift: Add first and
last properties to Array class

> Tools/LayoutTestRelay/LayoutTestRelay/OptionParser.swift:28
> +class OptionParser {

How hard would it be to use plain ol' getopt_long() from C instead?  Seems like
a lot of code to re-invent the wheel here, but I'm not sure how much code would
be require to wrap the old C wheel.

> Tools/Scripts/build-layouttestrelay:47
> +GetOptions(

You should check the result of GetOptions():

my $result = GetOptions([...]);

> Tools/Scripts/build-layouttestrelay:52
> +if ($showHelp) {

Then check for !$result here:

if ($showHelp || !$result) {

> Tools/Scripts/build-layouttestrelay:63
> +my $result = buildXCodeProject("LayoutTestRelay", $clean, XcodeOptions(),
@ARGV);

Don't need 'my $result' here if it is declared above.