Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: Map support constructor initialization with MapInitialEntries #2941

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pebrianz wants to merge 7 commits into AssemblyScript:main
base: main
Choose a base branch
Loading
from pebrianz:enhancement/map-initializer

Conversation

@pebrianz
Copy link
Contributor

@pebrianz pebrianz commented Aug 23, 2025
edited
Loading

  • Added new type alias MapInitialEntries<K, V> = { key: K, value: V }[]
  • Map constructor now accepts:
    • An array of { key, value } objects
    • Any array typed as MapInitialEntries<K,V>
  • This allows populating a Map in one step with strong typing.
  • Example:
 let myMap = new Map<string, i32>([ 
 { key: "a", value: 1 }, 
 { key: "b", value: 2 }, 
 ]);
 let init: MapInitialEntries<string, i32> = [ 
 { key: "x", value: 10 }, 
 { key: "y", value: 20 }, 
 ]; 
 let anotherMap = new Map<string, i32>(init);

Fixes #2940

  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

- Added new type alias `MapInitialEntries<K, V>` = { key: K, value: V }[]
- Map constructor now accepts:
 • An array of { key, value } objects
 • Any array typed as MapInitialEntries<K,V>
- This allows populating a Map in one step with strong typing.
- Example:
 let myMap = new Map<string, i32>([
 { key: "a", value: 1 },
 { key: "b", value: 2 },
 ]);
 let init: MapInitialEntries<string, i32> = [
 { key: "x", value: 10 },
 { key: "y", value: 20 },
 ];
 let anotherMap = new Map<string, i32>(init);
Comment on lines 79 to 83
if(entriesLength > 0) {
if(entriesLength >= this.entriesCapacity) this.bucketsMask = entriesLength;
this.rehash((this.bucketsMask << 1) | 1);

for(let i = 0; i < entriesLength; i++){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(entriesLength > 0) {
if(entriesLength >= this.entriesCapacity) this.bucketsMask = entriesLength;
this.rehash((this.bucketsMask << 1) | 1);
for(let i = 0; i < entriesLength; i++){
if(entriesLength > 0) {
if(entriesLength >= this.entriesCapacity) this.bucketsMask = entriesLength;
this.rehash((this.bucketsMask << 1) | 1);
for(let i = 0; i < entriesLength; i++){

@pebrianz pebrianz changed the title (削除) feat(Map): support constructor initialization with MapInitialEntries (削除ここまで) (追記) feat: Map support constructor initialization with MapInitialEntries (追記ここまで) Aug 23, 2025
Copy link
Contributor Author

the test rt/flags failed to compile, when pass type v128 to the map. it need simd to be enabled. Do you have any suggestion?

Copy link
Member

MaxGraey commented Aug 23, 2025
edited
Loading

For update all fixtures, try to run ASC_FEATURES="*" && npm run test:compiler -- --create

Copy link
Member

@CountBleck CountBleck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems acceptable, since we don't have tuples or iterables/iterators. (If/when we do add support for tuples, we would replace this mechanism as a breaking change.) I don't know if we should be concerned about the size increase in symbol.release.wat.

See additional comments below.


constructor() {
/* nop */
constructor(initialEntries: MapInitialEntries<K,V> = []) {
Copy link
Member

@CountBleck CountBleck Aug 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be nullable, and the default argument should be null instead of []. Tip: if you wrap the entire constructor body with if (initialEntries) {, you can avoid doing initialEntries!.

(I wonder if this change could be beneficial for code that never uses this feature, by making it easier for Binaryen to optimize)

private entriesCount: i32 = 0;

constructor(initialEntries: MapInitialEntries<K,V> = []) {
let entriesLength = initialEntries.length;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plz revert this cached entriesLength var and use instead initialEntries.length

if (initialEntries.length >= this.entriesCapacity) this.bucketsMask = initialEntries.length;
this.rehash((this.bucketsMask << 1) | 1);

for (let i = 0; i < initialEntries.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plz cache initialEntries.length into var

constructor() {
/* nop */
constructor(initialEntries: MapInitialEntries<K,V> | null = null) {
if (initialEntries) {
Copy link
Member

@MaxGraey MaxGraey Aug 25, 2025
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use early return:

Suggested change
if (initialEntries){
if (!initialEntries||!initialEntries.length)return;

This will reduce one level of ident

CountBleck and pebrianz reacted with thumbs up emoji
Copy link
Member

@CountBleck CountBleck Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad for suggesting that

Copy link

This PR has been automatically marked as stale because it has not had recent activity. It will be closed in one week if no further activity occurs. Thank you for your contributions!

Copy link

github-actions bot commented Nov 1, 2025

This PR has been automatically closed due to lack of recent activity, but feel free to reopen it as long as you merge in the main branch afterwards.

Copy link
Member

Just gonna keep this open so it doesn't get buried too quickly

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

Reviewers

@MaxGraey MaxGraey MaxGraey left review comments

@CountBleck CountBleck Awaiting requested review from CountBleck

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow Map to be initialized with an iterable

AltStyle によって変換されたページ (->オリジナル) /