Skip to content

Implementation of Properties.#12097

Closed
12345swordy wants to merge 13 commits intodlang:masterfrom
12345swordy:Properties
Closed

Implementation of Properties.#12097
12345swordy wants to merge 13 commits intodlang:masterfrom
12345swordy:Properties

Conversation

@12345swordy
Copy link
Contributor

@12345swordy 12345swordy commented Jan 4, 2021

I have rebase @JinShil PR on this and added some to the test.

TODO:

  • disallow functions that is mark property from returning ref.
  • Implement binary operators successfully.
  • Implement unary operators successfully.
  • Disallow parameters for property functions
  • write a dip on this for justification on modifying properties.

cc @thewilsonator need a "need DIP" and "Spec PR" red tag for this.

@PetarKirov PetarKirov added Review:Needs Changelog A changelog entry needs to be added to /changelog Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org Review:Pending DIP Approval labels Jan 4, 2021
@PetarKirov
Copy link
Member

After a few years of experience with other languages I've come to appreciate the dedicated syntax that some of them provide (e.g. C#, Dart, JavaScript), as opposed to D's bare-bones approach.
That said, think we can have a two step process:

  1. Define the semantics using existing function syntax + @property
  2. Add syntax sugar on top. The expression-bodied members syntax would be a very nice win.

This could be done via a single, or two separate DIPs.

@adamdruppe
Copy link
Contributor

adamdruppe commented Jan 4, 2021 via email

* the rewritten expression if the procedure succeeds, an `ErrorExp` if the
* and error is encountered, or `null` if `e.e1` is not a `@property` function.
*/
Expression SemanticProp(BinAssignExp e, Scope* sc)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this called binSemanticProp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It used to be called that. It was initially to be implemented for binary operators only, which I plan to expand father than that. That may change in future.

@tgehr
Copy link
Contributor

tgehr commented Jan 5, 2021

Nice, but why those?

disallow functions that is mark property from returning ref.
Disallow parameters for property functions

@12345swordy
Copy link
Contributor Author

12345swordy commented Jan 5, 2021

Nice, but why those?

disallow functions that is mark property from returning ref.
Disallow parameters for property functions

That is part of the design I have in mind. It meant to be used as a variable, with the key difference is that you can't obtain the address of it directly nor can you pass it to a ref parameter of a function. All write and read access of the private variable outside the module must go through the functions themselves. No exceptions.

I will explain this further in detail when I write up the dip of this.

Edit: This is my major influence. https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/properties

@RazvanN7
Copy link
Contributor

RazvanN7 commented Jan 6, 2021

@12345swordy Can you please link the original PR so that reviewers can easily keep track of the discussions?

@12345swordy
Copy link
Contributor Author

@12345swordy Can you please link the original PR so that reviewers can easily keep track of the discussions?

#7079

@12345swordy
Copy link
Contributor Author

12345swordy commented Jan 8, 2021

Erm, it appears I didn't re base correctly.... Any tips?

NM

@12345swordy
Copy link
Contributor Author

OK, after much discussion on discord, I decided to remove the "disallow functions that is mark property from returning ref" restriction.

@12345swordy
Copy link
Contributor Author

Finished 80% implementation of binary operators for properties, the other 20% involves an edge case that I am not sure that it should compile.
Going to start working on unary operators now.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @12345swordy! 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 coverage diff by visiting the details link of the codecov check)
  • 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

Auto-close Bugzilla Severity Description
21658 normal Debugger is reading enums with EnumBaseType incorrectly

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#12097"

@12345swordy
Copy link
Contributor Author

@RazvanN7 are you available at slacks or at discord? I need assistance on finishing this.

@12345swordy
Copy link
Contributor Author

It is done for review. Need to rebase it, now.

testTInt();
testTString();
testFreeFunctions();
} No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

Add a newline to conform with POSIX 3.206 .

testSideEffectsInt();

testSideEffectsString();
} No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@RazvanN7
Copy link
Contributor

@12345swordy Sorry for the late reply. Yes, I am available on slack. Whatever questions you may have, feel free to ask.

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

Labels

Review:Needs Changelog A changelog entry needs to be added to /changelog Review:Needs Rebase Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org Review:Needs Work Review:Pending DIP Approval Review:stalled Severity:Bug Fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants