Conversation
|
This change allows running the following tests: This change does not change the handling of empty modules in the library. |
| if (errors.size() == 1 && errors[0].error_level == ErrorLevel::Warning && | ||
| errors[0].message == "empty module") { |
There was a problem hiding this comment.
This seems a little hacky to generate the warning and then remove it. Is this worth the benefit? I.e. do we have such modules in the wild that we want to support without warnings?
There was a problem hiding this comment.
What do you suggest instead?
This affect spec tests (Quoted modules) only.
There was a problem hiding this comment.
Is there some way we could simple not generate the "empty module" warning at all in the case that annotation is present? I assume that is not simple otherwise you would have done it already?
There was a problem hiding this comment.
Yes. The next token functions silently consumes the unknown annotations. Adding a bool which is set if an annotation is encountered seems the easiest solution, but this is a warning, and probably useful in the general case. Warnings converted to errors by the spec test executor for some reason. We could also ignore warnings though.
There was a problem hiding this comment.
I think the empty module warning does seem reasonable, so maybe finding a way to suppress it in the spec runner is the best way to do. Presumably its OK if the spec tests contain warnings?
There was a problem hiding this comment.
What are the purpose of warnings btw? We only have 3 types of warnings at the moment: empty module, empty script, and custom section reading error in binary reader.
As for me warnings represent those cases, which are valid according to the specification, but still useful to tell the user a possible issue. From this perspective, spec tests might contain warnings, and they are still valid.
To reduce human error, I would only suppress as few warnings as possible. Contents of the spec tests should be ok, but we might make errors when processing them. Warnings could help to reveal that.
This branch has not been deployed
2 participants
No description provided.