Skip to content

Conversation

@MaxGraey
Copy link
Member

No description provided.

@MaxGraey MaxGraey requested a review from dcodeIO September 24, 2019 07:36
@MaxGraey MaxGraey requested a review from dcodeIO September 25, 2019 14:51
@dcodeIO
Copy link
Member

dcodeIO commented Sep 26, 2019

LGTM :) Might be a good opportunity to also think about the dataStart name, which somewhat made sense while there was data and dataLength, but now is a bit unintuitive. I don't have a good idea for a better name, though.

@MaxGraey
Copy link
Member Author

MaxGraey commented Sep 26, 2019

I guess better leave old name for dataStart because it's already used a lot in external third-party source codes. Also it's unsafe and don't use in safe-context which I guess will be standard de-facto in future.

@dcodeIO
Copy link
Member

dcodeIO commented Sep 26, 2019

There is a chance that removing data and dataLength breaks these cases anyway, hence the suggestion. Isn't super important to rename it ofc, but if we want to do it I'd say that this is the PR - or we'll never do it :)

@MaxGraey
Copy link
Member Author

MaxGraey commented Sep 26, 2019

Yeah. We could add something like:

class ArrayBufferView {
  ...
  @unsafe readonly rawBuffer: usize;
  ...

  @inline function dataStart(): usize {
    WARNING("dataStart was deprecated. Please use 'rawBuffer' instead");
    return this.rawBuffer;
  }
}

@MaxGraey
Copy link
Member Author

@dcodeIO what about rawBuffer?

@dcodeIO
Copy link
Member

dcodeIO commented Sep 27, 2019

Thinking about it a little more, it's "the unsafe pointer to the start of the data in the backing buffer", so dataStart isn't that bad maybe, with only dataStartPtr or maybe a short unsafePtr being more concise but perhaps not worth the trouble I guess.

@dcodeIO dcodeIO merged commit bd78683 into AssemblyScript:master Sep 29, 2019
@dcodeIO
Copy link
Member

dcodeIO commented Sep 29, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants