FuelLabs
Commas in IR instructions
We need to review and see if the grammar needs them to avoid ambiguity in the parser. It looks a little cluttered to me but if removing them makes the parser harder then it's worth keeping them.
@otrho should we just close this? Seems a bit redundant to be discussing it or even putting in the effort into removing the commas.
I wouldn't mind spending a morning assessing this, hence the self assignment a couple of weeks ago. The commas do seem to add clutter.
@mohammadfawaz (and @vaivaswatha) I took a look at this and have found the commas are generally needed, especially for things like types which can be ptr t, i.e., multiple words. The one place for which they stick out is metadata and I think that's what this issue was originally created for (or in response to, generally).
But still, there can be ambiguity when parsing metadata without commas, at least in it's current form. I'm not 100% happy with it though, so I wonder what you think. Here's an example where there might be ambiguity. A const instruction is:
v0 = const u64 42
But when parsing immediates we allow a span, but also each instruction could have a span. So, in this case we wouldn't have both, but the parser isn't smart enough to know. I'm pretty sure there would be places (other than const where this is a valid problem though).
v0 = const u64 42 !2 // Would attach the span MD to the 42.
v1 = const u64 42, !3 // Would attach the span MD to the whole const instruction.
v2 = const u64 42 !4, !5 // Would attach both 'correctly' although this wouldn't be generated like this by Sway.
This is only unambiguous due to the comma.
But I'm thinking the instruction MD shouldn't necessarily go at the end of the line. It kinda makes sense, but an alternative might be to put the instruction MD after the operation name:
v0 = const !2 u64 42 !3 // !2 for the instruction, !3 for the 42.
Better? Dunno.
So, is it worth changing things for the sake of 'clutter' even though a solution might make the instructions less readable? Is there a better place or scheme for this?
And perhaps since my next task now is to generalise MD #1906 am I going to find other problems anyway? We will potentially have many MD tags attached to many different parts of the IR, as in a span, a state index, an other annotation, all collectively attached to a single IR item. So a new syntax might be required to avoid ambiguity in that case anyway.
should we just close this? Seems a bit redundant to be discussing it or even putting in the effort into removing the commas.
And if you guys don't have any immediately obvious thoughts then no, this probably isn't actually worth worrying about.
And perhaps since my next task now is to generalise MD #1906 am I going to find other problems anyway? We will potentially have many MD tags attached to many different parts of the IR, as in a span, a state index, an other annotation, all collectively attached to a single IR item. So a new syntax might be required to avoid ambiguity in that case anyway.
I think it would be wise to just wait until this is done, especially since there is no urgency.
#2394 has now made it so there can be only one MD tag per item, so the commas are probably redundant. So I won't close this quite yet.