Add more support to extended names section. #1659
Conversation
I noticed we lacked support here while debugging #1651.
| @@ -1,6 +1,10 @@ | |||
| ;;; TOOL: run-roundtrip | |||
| ;;; ARGS: --debug-names | |||
| ;;; ARGS: --debug-names --enable-bulk-memory | |||
There was a problem hiding this comment.
Do we want a new test with this flag and the additions below? Otherwise we only test with this flag enabled.
There was a problem hiding this comment.
The reason I need to add this flag is that naming elem sections isn't possible without bulk memory enabled. If we added another test it would looks just the same but I couldn't include the elem section name (its a parse error without bulk memory).
WDYT, should add a the second (module duplicate) test? I'm happy to to if you think its worth it.
There was a problem hiding this comment.
Ic, I can't imagine many cases where the duplicate case will be useful, since bulk-memory flag shouldn't affect the way we handle names for other sections. LGTM to not add a test. Thanks for explaining!
| return Result::Ok; | ||
| } | ||
| if (index >= module_->tables.size()) { | ||
| PrintError("invalid table index: %" PRIindex, index); |
There was a problem hiding this comment.
Turns out that templatizing the error message here is not easy in C++... https://stackoverflow.com/questions/1826464/c-style-strings-as-template-arguments
I'll keep trying but will probably land as is and followup with a templetization.
There was a problem hiding this comment.
Could we template on a typename T, then pass the entire string "Invalid table index: %" PRIindex as an arg, and maybe let some deduction/conversion happen?
Follow-up (with a TODO to indicate what we would like to do here) would be totally fine.
I noticed we lacked support here while debugging #1651.