Bug 163919

I implemented std::expected in WTF, and plan to use it to improve error messages soon.

Created attachment 297052 [details] WIP WIP patch, all is ready for comments except: - WasmB3IRGenerator part which I still have to do. - Tests which need to be updated. I'll also wait for Saam and Keith's patches for Element + Global to land before landing this change. I'm thinking about punting good validator error messages for now. It would be pretty much the same thing as I've already done (see fail() helper in WasmParser.h).

Oh and before it gets asked, same answer as Yusuke on auto -> Result syntax: https://bugs.webkit.org/show_bug.cgi?id=164757#c27 :-)

Comment on attachment 297052 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=297052&action=review This approach seems reasonable to me. > Source/JavaScriptCore/wasm/WasmFunctionParser.h:81 > + if (UNLIKELY(!addResult)) \ > + return addResult.getUnexpected(); \ > + } while (0) Indentation looks off here. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:323 > + return PartialResult(); Why not "return { }" for this and all the other "return PartialResult()" > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:384 > + WASM_PARSER_FAIL_IF(!parseUInt8(opcode), "can't get ", segmentNumber, "th Data segment's opcode"); JSC's parser also adds a macro that would be named like so in your naming scheme WASM_PARSER_FAIL_IF_FALSE() and then this line of code could be WASM_PARSER_FAIL_IF_FALS(parseUInt8 ...) I'm not sure if it's better or not, but it's worth mentioning. > Source/JavaScriptCore/wasm/WasmValidate.cpp:197 > + if (UNLIKELY(condition != I32)) > + return UnexpectedType<Result::ErrorType>("validation failed: select condition must be i32"); Seems like you should have a macro for a condition failing here to return a string wrapped in UnexpectedType<Result::ErrorType> Also these can be ASCIILiteral

Created attachment 297102 [details] WIP This WIP addresses the other comments, and doesn't yet deal with B3 generation, improving the validation messages, nor the tests. > > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:384 > > + WASM_PARSER_FAIL_IF(!parseUInt8(opcode), "can't get ", segmentNumber, "th Data segment's opcode"); > > JSC's parser also adds a macro that would be named like so in your naming > scheme > WASM_PARSER_FAIL_IF_FALSE() > and then this line of code could be > WASM_PARSER_FAIL_IF_FALS(parseUInt8 ...) > > I'm not sure if it's better or not, but it's worth mentioning. I think the positive one reads more easily: "fail if this happens" instead of "fail if this doesn't happen". I read this as "fail if you can't parse uint8". It's nicer in other places too: "fail if this is empty" and "fail if the type isn't `void`". > > Source/JavaScriptCore/wasm/WasmValidate.cpp:197 > > + if (UNLIKELY(condition != I32)) > > + return UnexpectedType<Result::ErrorType>("validation failed: select condition must be i32"); > > Seems like you should have a macro for a condition failing here to return a > string wrapped in UnexpectedType<Result::ErrorType> > > Also these can be ASCIILiteral Yeah I'll move it to the same thing as the rest of the patch, so it'll be much nicer.

Created attachment 297149 [details] WIP This is almost good to go, I need to update the tests.

Created attachment 297159 [details] WIP A few tests, more to come. I'm half-online, will add others later.

Created attachment 297168 [details] patch for review The patch is ready for review, but is still blocked on two patches which I'll get to tomorrow. I'll also add a changelog.

Comment on attachment 297168 [details] patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=297168&action=review r=me > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:152 > + return fail(__VA_ARGS__); \ Style: Indentation is wrong here. > Source/JavaScriptCore/wasm/WasmFunctionParser.h:268 > + WASM_PARSER_FAIL_IF(!parseVarUInt32(alignment), "can't get store alignment"); > + WASM_PARSER_FAIL_IF(!parseVarUInt32(offset), "can't get store offset"); > + WASM_TRY_POP_EXPRESSION_STACK_INTO(value, "store value"); > + WASM_TRY_POP_EXPRESSION_STACK_INTO(pointer, "store pointer"); > + WASM_TRY_ADD_TO_CONTEXT(store(static_cast<StoreOpType>(op), pointer, value, offset)); Style: It seems weird that one macro error requires "can't <blah" and the other doesn't > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:402 > + WASM_PARSER_FAIL_IF(exp.kindIndex, "can't export Table ", exp.kindIndex, " only one Table is currently supported"); Nit: I think this is easier to understand if you say only a zero index table is allowed. Saying only one doesn't say what index is expected.

Created attachment 297228 [details] patch Merge, address comments, ChangeLog.

Attachment 297228 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:76: static_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:77: static_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:78: static_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:79: static_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:86: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/WasmParser.h:89: Do not use 'using namespace FailureHelper;'. [build/using_namespace] [4] Total errors found: 6 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 297228 [details] patch Clearing flags on attachment: 297228 Committed r209880: <http://trac.webkit.org/changeset/209880>

All reviewed patches have been landed. Closing bug.