Conversation
src/wasm-validator.h
Outdated
|
|
||
| // Print anything that can be streamed to an ostream | ||
| template <typename T> | ||
| template <typename T, |
There was a problem hiding this comment.
No reason, just leftover from a previous edit. de-intented.
src/wasm-validator.h
Outdated
| template <typename T> | ||
| template <typename T, | ||
| typename std::enable_if< | ||
| !std::is_base_of<Expression, typename std::remove_pointer<T>::type>::value |
There was a problem hiding this comment.
this is starting to feel rather complicated. perhaps we should rethink the printing code?
e.g. in llvm I believe they support printing of objects, not pointers, which maybe makes things simpler (so you need to do cerr << *x instead of just x etc.).?
There was a problem hiding this comment.
I do like that idea, but in this case I think it wouldn't change much; it would just remove the need for the remove_ptr bit.
There was a problem hiding this comment.
Well, I'm hoping with a larger refactoring we could remove the need for all of this printing code. Perhaps we can leave it for another PR. But iiuc, the issue here is to pass the option of printing the types, then we could have an API like this:
std::cerr << FullTypePrinter(expr)
which would print it with the full type. And so we could end up with just cerr << [..] for everything, and without these new templates and function calls?
There was a problem hiding this comment.
I think with something like that you'd have the same problem, i.e. you want it to be transparent to the caller (e.g. a template like fail) whether you are dealing with an expr (in which case you want to wrap it with FullTypePrinter) or not (in which case you don't). So IIUC to do that you'd have to specialize/overload/templatize FullTypePrinter similarly.
There was a problem hiding this comment.
We could do the wrapping in the calls to fail but yeah, maybe that's no better.
src/wasm/wasm-validator.cpp
Outdated
| switch (curr->type) { | ||
| case i32: | ||
| case i64: | ||
| case unreachable: |
There was a problem hiding this comment.
other code uses {, } when body is on another line, also in switch cases. How about just putting it on the same line?
There was a problem hiding this comment.
Like case i32: case i64: case unreachable: break;?
There was a problem hiding this comment.
I'd suggest
case i32:
case i64:
case unreachable: break;
or
case i32:
case i64:
case unreachable: {
break;
}
since both appear elsewhere.
There was a problem hiding this comment.
I really don't like option A, so I'll do B
src/wasm/wasm-validator.cpp
Outdated
| validateMemBytes(curr->bytes, curr->type, curr); | ||
| shouldBeEqualOrFirstIsUnreachable(curr->ptr->type, i32, curr, "AtomicRMW pointer type must be i32"); | ||
| shouldBeEqualOrFirstIsUnreachable(curr->value->type, curr->type, curr, "AtomicRMW result type must match operand"); | ||
| switch (curr->type) { |
There was a problem hiding this comment.
Nit: helper function? bool isTypeIntOrUnreachable(WasmType t)? The switch/default/fail is just odd enough to be not totally obvious.
Or bool shouldBeIntegerOrUnreachable(WasmType, Expression*, const char*) to match the shouldBeEquals
3 participants
Also fix cases where
fail()had the arguments backwards. Wasn't an error because lol templates.Also fix
printModuleComponenttemplate to SFINAE onExpression*so we properly get the specialized version.