Skip to content

Comments

Type-Safe template wrapper for AA#7924

Merged
wilzbach merged 1 commit intodlang:masterfrom
marler8997:aaTemplate
Feb 28, 2018
Merged

Type-Safe template wrapper for AA#7924
wilzbach merged 1 commit intodlang:masterfrom
marler8997:aaTemplate

Conversation

@marler8997
Copy link
Contributor

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@marler8997 marler8997 force-pushed the aaTemplate branch 2 times, most recently from a8dcd88 to 9d42f42 Compare February 20, 2018 17:16
Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Nice. I was about to add this too, but you beat me to it :)

V opIndex(const(K) key)
{
return cast(V)dmd_aaGetRvalue(aa, cast(void*)key);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding opIndexAssign too?

    auto opIndexAssign(V value, K key)
    {
        auto ptr = dmd_aaGet(&aa, cast(void*) key);
        *ptr = cast(void*) value;
        return *ptr;
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently no code uses it this way and I don't anticipate code ever doing this.

aa[key] = value;

This being the more "intuitive" usage, I have to assume that Walter deliberately chose not to support this.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it's just because it wasn't implemented? maybe ask @WalterBright ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's something worth bugging him about. Currently there's no use for it. We can always add it later if a use comes to light.

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep in mind that the current code base was translated from C+ (not a typo). I don't think there was any deliberate effort by Walter to avoid it.

Copy link
Contributor Author

@marler8997 marler8997 Feb 21, 2018

Choose a reason for hiding this comment

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

I'm also basing this on comments he made about one of @ibuclaw's PRs where he made a change that ended up causing multiple lookups of the same key. The current implementation uses getLvalue which allows the developer to do multiple operations on a value with only one lookup. I think the Walter designed the API to encourage this behavior, and adding opIndexAssign would provide a way to circumvent it, making it easy for developers to accidentally write code that performs multiple lookups that aren't necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's Walter's comment: #7696 (comment)

{
return cast(V)dmd_aaGetRvalue(aa, cast(void*)key);
}
} No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a public example:

///
unittest
{
    AssocArray!(string, int) aa;

    assert(aa["foo"] == 0);
    aa["foo"] = 2;
    assert(aa["foo"] == 2);
    assert(aa.length == 1);
}

@marler8997 marler8997 force-pushed the aaTemplate branch 7 times, most recently from 16d7b3d to b51e83f Compare February 20, 2018 22:34
@JinShil JinShil added the Severity:Refactoring No semantic changes to code label Feb 23, 2018
@wilzbach wilzbach added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Feb 23, 2018
Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Nice. I added this to the "no objection -> merge" queue.

@wilzbach wilzbach merged commit 9c6455f into dlang:master Feb 28, 2018
@nordlow
Copy link
Contributor

nordlow commented Feb 28, 2018

Whats the purpose with this addition?

@marler8997
Copy link
Contributor Author

marler8997 commented Feb 28, 2018

I thought the purpose was self evident from the change (and the title)...what do you think?

@nordlow
Copy link
Contributor

nordlow commented Feb 28, 2018

  1. Is the long-term goal with this change to make the default AA-implementation user-configurable?
  2. Without the reading the code...what kinds of extra type-safety does this change introduce?

@marler8997
Copy link
Contributor Author

No long term goal in mind for this change, the change is its own end goal, a type safe template wrapper around AA.

@wilzbach
Copy link
Contributor

This has nothing to do with druntime. It's about introducing more type safety into the dmd codebase which uses its own AA-implementation back from the C++ times.

I think what you are looking for is something like dlang/druntime#1282

@nordlow
Copy link
Contributor

nordlow commented Feb 28, 2018

Ahh, now I get it. Thanks, @wilzbach, I thought I was looking at a druntime PR... my mistake.

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

Labels

Merge:72h no objection -> merge The PR will be merged if there are no objections raised. Severity:Refactoring No semantic changes to code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants