RFC: Only track byte offsets in codemap and related modules

[kevina]

I do not believe it is really necessary to keep track of both the character and byte offset in codemap and related modules.

Spans and everything else should be using the byte offsets. The only place where character offsets are likely needed is when reporting per line column offset, in that case it should be fairly easy to convert the byte offset from the start of the line to a character offset.

Removing all character offset tracking will likely simply the code somewhat.

Right now when trying to work with code in codemap I'm never quite sure if the offset is a byte or a character offset. Furthermore, at one point I had one offset in a byte offset and the other in a character offset (which is what prompted me to create this report).

[brson]

I'm all for simplifying as long as we can continue to derive the character offsets where we need it. The codemap originally stored byte offsets but character offsets were added so that errors for multi-byte unicode source displayed correctly.

[kevina]

Do we need them anywhere other than column offsets?

[brson]

I think it's needed to display the column offset in error messages, and to place the underlines (^~~~~~).

How would you find the character offset from the byte offset? Right now we don't have access to the source at error reporting time. diagnostic::highlight_lines handles this by re-opening the source file, but that doesn't work when the source comes from stdin.

[kevina]

From what I can tell the lexer/parser reads in the entire file into a string anyway. Is there some reason it doesn't keep the string around?

Is it because you worried about it taking up unnecessary memory?

[kevina]

Note: on IRC brson and jdm agreed that keeping the source string around is a good idea, as it will avoid having to reread the file in highlight_lines and will also work in the case of stdin.

This will need to be done before this issue can be addressed.

[kevina]

So I spent a night trying to address this issue and didn't get very far, and now I'm not sure if it is still worth it.

In fact the byte offsets in the filemap are not even used as I found a rather nasty bug, see 448c71b.

At this point I think the commit to keep the source file around (2a07f4c) and the bug fix commit (448c71b) should go in. @brson do you agree? The rest can come another day.

[brson]

I agree that both those commits look good, but something changed overnight and they don't apply cleanly now. Can you rebase? I also would really like to see gather_comments_and_literals use this, as it would basically double the justification for making the change. I take it we will be able to remove the byte offsets? That seems like just as good an outcome - maybe better since we won't have to do any conversions - than the original goal.

[kevina]

@brson: actually the byte offset is needed, it is just that lookup_char_pos is not used, but it might be useful so I don't want to remove it just yet.

I will rebase those two commits and create a pull request

[kevina]

@brson: I took a brief look at gather_comments_and_literals and the conversion didn't seam obvious, I may have another look later and if I convert it I will open a separate pull request.

[nikomatsakis]

Seems that all has been done here that will be done for the moment.