Skip to content

Conversation

idanarye
Copy link
Contributor

Here is the forum discussion about this module suggestion: http://forum.dlang.org/thread/fofbrlqruxbevnxchxdp@forum.dlang.org

And here are the ddocs: http://someboddy.github.io/phobos/ddocs/for-idioms/idioms.html

Currently 2 idioms are implemented:

  • Properties - Generate property getter and setter accessors member fields.
  • Singleton - Turns the class into a singleon.

Properties usage:

class Foo
{
    mixin Properties!q{
        @Asserting("a < 10") int x;
        @Enforcing("a <= x") int y;
    };
}

This will create in the class Foo two property fields - x and y, where x's setter will assert that any new value is smaller than 10, and y's setter will check that any new value is smaller or equal to the current value of x and throw a PropertyException otherwise.

*Singleton usage:

class Foo
{
    mixin ThreadLocalSingleton;
}
class Bar
{
    mixin SharedSingleton;
}
class Baz
{
    mixin _GSharedSingleton;
}

This will turn Foo into a thread local singleton(each thread will have it's own instance), and Bar and Baz into global singletons(all threads access the same instance). Both Bar and Baz will use David Simcha's and Alexander Terekhov's low lock singleton implementation approach for initialization, but while Bar will use D's shared to ensure thread safety with the usage of the instance, Baz will have thread safety enforced by the idiom - the users will have to do it themselves.

idanarye added 3 commits May 20, 2013 18:32
Currently 3 idioms are implemented:

* Properties -           Generate property getter and setter accessors
                         for member fields.
* LowLockSingleton -     Turns the class into a singleon, using David
						 Simcha's and Alexander Terekhov's low lock
						 singleton implementation approach.
* ThreadLocalSingleton - Turns the class into a thread local singleton.
Also renamed `LowLockSingleton` to `__GSharedSingleton` to make it clear
that it's instance object is not thread safe.
@Poita
Copy link
Contributor

Poita commented Jun 5, 2013

From the spec: "Identifiers starting with __ (two underscores) are reserved." Please remove all of these.

std/idioms.d Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Meant to say "Enforcing will be..."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I guess your way does make more sense

@idanarye
Copy link
Contributor Author

idanarye commented Jun 5, 2013

@Poita for some reason I thought __ was for marking internally used identifiers...

Also fixed some mistakes in the documentation.
@ghost
Copy link

ghost commented Oct 24, 2013

Hmm.. I really don't know how often these would be used. Maybe now that we have dub this should belong into a separate library.

@dejlek
Copy link

dejlek commented Jan 31, 2014

Should this really be this complicated? Hundreds of lines of code for something as simple as Singleton?

@idanarye
Copy link
Contributor Author

@dejlek There are 3 versions of the singleton, each is about 75 concrete LoC. The original low-lock algorithm is 17 LoC(22 when converted to Phobos coding format standards), but I had to add more code to support explicit instantiation and checking for instantiation and to make it generic enough to put in a library mixin, so I think 75 LoC is reasonable.

The rest of the 600 lines that compose the singleton section of this module are ddocs and unittests, and it's really hard to have too many of those...

@andralex
Copy link
Member

Not sure how to go about this. First off, Singleton has become almost universally vilified even in the Design Patterns community, so starting a module with it doesn't bode well :o). I'm tempted to recommend doing away with it entirely.

Properties are nice. Probably too little to start a new module without a clear charter. BTW "idioms" is a bit too general (admittedly "std.algorithm" is guilty of that, too). I'm unclear what would go in there to flesh it out.

@kiith-sa
Copy link
Contributor

I think having singletons in Phobos is a bad idea.

The properties may be a good idea, but I don't like the API.

String mixin in API can provide a lot of space for a user to shoot themselves in the foot.

I think a better API would be something along the lines of this:
(but... it's still ugly, and making it non-ugly makes it long)
(and it wouldn't work, because lambdas in class bodies don't work even if inside template params)
(and it depends on a naming convention which is not nice either, but removes a "name" template param)

int x_;
mixin AssertingProp!(a => a < 10, x_);

Still, it would not be useful all that often, because e.g. often I want a getter but not a setter and vice
versa, most of the time I want some of @safe pure nothrow const @nogc, etc.

And if you added all of that, you'd end up mostly reimplementing properties, (although you could
probably figure out @safe pure nothrow etc. in the mixin for some cases).

@mihails-strasuns
Copy link

As I have stated in the initial newsgroup thread, I am against of inclusion of this module into Phobos in its current shape. It includes some arguable idioms and wide usage of string mixin based API is not type-safe enough for inclusion into standard library.

Even if it considered for inclusion into Phobos, it must undergo formal review process because of very controversial content.

Unless someone is willing to take this over and go through the pains of review queue I'd propose to close this PR.

@mihails-strasuns
Copy link

@idanarye are you going to proceed with formal review of this? If yes, please update the docs and write me an e-mail mentioning when you will have enough time to respond the reviewer comments regularly (not before current review ends anyway).

Otherwise I am going to close it.

@idanarye
Copy link
Contributor Author

@Dicebot seeing that everyone are pretty much against it, I guess it's better to just close it.

@idanarye idanarye closed this Jul 23, 2014
@mihails-strasuns
Copy link

Sorry if it was discouraging :(

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.

7 participants