Closed Bug 1253877 Opened 4 years ago Closed 4 years ago

Baldr: print missing text labels in resolving phase

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(1 file)

No description provided.
Attachment #8727125 - Flags: review?(mbebenita)
Comment on attachment 8727125 [details] [diff] [review]
better-text-errors

Review of attachment 8727125 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, and nice to have. We could improve on this by also printing the text position of the label that fails to resolve.

::: js/src/asmjs/WasmText.cpp
@@ +3179,3 @@
>          return false;
>      }
> +    bool fail(const char* kind, WasmName name) {

NIT, since this method is specific to labels not being found, maybe we could call it something else. But I can't really think of a good name right now.
Attachment #8727125 - Flags: review?(mbebenita) → review+
> Looks great, and nice to have. We could improve on this by also printing the
> text position of the label that fails to resolve.

That's a good idea, but probably in a followup patch since it involves additionally stashing line/column offset in WasmName.

> NIT, since this method is specific to labels not being found, maybe we could
> call it something else. But I can't really think of a good name right now.

Good point; I'm thinking failResolveLabel().
https://hg.mozilla.org/mozilla-central/rev/2c9739f8236f
https://hg.mozilla.org/mozilla-central/rev/56aed3cbc3c0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.