Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

Makes core.sync.mutex.Mutex usable in modern code #1728

Merged
merged 11 commits into from
Jan 26, 2017

Conversation

PetarKirov
Copy link
Member

@PetarKirov PetarKirov commented Dec 28, 2016

Makes core.sync.mutex.Mutex usable in modern @safe nothrow @nogc D code:

  1. Make Mutex.this() / this(Object) / ~this() nothrow and @nogc
  2. Add shared overloads to Mutex.this, Mutex.lock, Mutex.unlock and Mutex.tryLock
  3. Add tryLock_nothrow variant of Mutex.tryLock - for use in nothrow @nogc code, similar to Mutex.lock_nothrow and Mutex.unlock_nothrow.
  • TODO: Add unittests.

Please review commit by commit, as it would be more clear why each change is made.

@PetarKirov PetarKirov changed the title Add attributes to core sync mutex Makes core.sync.mutex.Mutex usable in modern @safe nothrow @nogc code Dec 28, 2016
@PetarKirov PetarKirov changed the title Makes core.sync.mutex.Mutex usable in modern @safe nothrow @nogc code Makes core.sync.mutex.Mutex usable in modern code Dec 28, 2016
@PetarKirov PetarKirov force-pushed the add-attributes-to-core-sync-mutex branch 3 times, most recently from b403473 to fb291b4 Compare December 28, 2016 21:22
Copy link
Member

@jmdavis jmdavis left a comment

Choose a reason for hiding this comment

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

Aside from the comments, I think that this looks good. Long term, I would think that we'd want to force shared on the whole API and deprecate the thread-local functions, but it's better to wait at least a release before doing that so that it's possible for folks to build their code for both the last release and master without getting a bunch of deprecation warnings that they can't fix.

throw new SyncError( "Unable to initialize mutex" );
scope(exit) pthread_mutexattr_destroy( &attr );
!pthread_mutexattr_init(&attr) ||
assert (0, "Unable to initialize mutex");
Copy link
Member

Choose a reason for hiding this comment

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

core.internal.abort would be closer to the previous functionality, since it would still print out in release mode. And given how incredibly confusing this failure would be if it were to ever occur without an error message, it would probably be better to use abort in all the cases in this commit where you use assert(0). And it'll work with @nogc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -499,8 +499,11 @@ int pthread_key_delete(pthread_key_t);
int pthread_mutex_destroy(pthread_mutex_t*);
int pthread_mutex_init(pthread_mutex_t*, pthread_mutexattr_t*) @trusted;
int pthread_mutex_lock(pthread_mutex_t*);
int pthread_mutex_lock(shared(pthread_mutex_t)*);
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make more sense to just leave the C functions as-is and cast away shared when calling them? After all, shared does not exist in C. I'm surprised that this even works. It's also pretty weird to have shared on only some of them when they're all clearly in the same boat.

Copy link
Member Author

@PetarKirov PetarKirov Jan 13, 2017

Choose a reason for hiding this comment

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

I deliberately made that change because it bothered me when porting C and C++ code to D, which was directly using the platform specific APIs. Especially when porting a C project where there were absolutely zero abstractions and I needed to add casts to almost every function, since almost every function used pthread_mutex directly and I wanted to use shared instead of __gshared.

I'm surprised that this even works

You shouldn't be, if you consider that strchr works the same way in C++. In C all symbols (including functions) are mangled the same as their name (give or take an underscore at the beginning). That allows (sometimes correctly and sometimes not) to map multiple overloads from C++ or D to the same C function. (Note: some C++ compilers allow C function overloading but require vendor specific extensions similar to D's pragma (mangle).)

Also consider that in D you can overload extern (C) functions on attributes like nothrow. Especially useful for function pointer parameters.

Copy link
Member Author

@PetarKirov PetarKirov Jan 14, 2017

Choose a reason for hiding this comment

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

It's also pretty weird to have shared on only some of them when they're all clearly in the same boat.

Not all of them can be called concurrently on the same object. For example pthread_mutex_destroy is not thread-safe - it should be called only when the (conceptual) ref count of the mutex drops to 1. Also removing the non-shared overloads would be breaking change.

Functions like pthread_rwlock_*lock should also have shared overloads, but I prefer to leave this for another day.

Copy link
Member

Choose a reason for hiding this comment

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

yah shared is good here

Copy link
Member

Choose a reason for hiding this comment

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

BTW: LDC does not allow multiple definitions that mangle to the same name, so this will probably come up again when updating druntime in LDC. Not sure whether that restriction should be lifted in LDC, though.

Copy link
Member Author

@PetarKirov PetarKirov Jan 14, 2017

Choose a reason for hiding this comment

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

Oh yeah, if I remember correctly that was changed recently. The correct way to go about that in LDC, in my opinion, is to allow overloading if and only if all overloads have the same ABI as should be the case here.

To be pedantically correct, those functions should have been declared as taking shared parameters in the first place, but leaving only the shared versions would be a breaking change now.

Copy link
Member

Choose a reason for hiding this comment

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

If that's the approach to take, then pretty much every C/C++ function should have shared parameters, because in reality, that's what they are. We just usually ignore that fact, because it usually doesn't matter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just like const prevents accidental mutation, shared guards against accidental use of non-thread-safe functions on shared data. Only functions that are proven to operate in a thread-safe way on shared data should be marked as shared.
In general, when we don't know whether a function is thread-safe we should not call it on shared data without sufficient synchronization and therefore we should not mark any of its parameters as shared.

Copy link
Member

Choose a reason for hiding this comment

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

There is zero guarantee that operating on something that's shared is thread-safe. If anything, it's the opposite. shared means that the data is not thread-local. So, proper synchronization is required. To be perfectly correct, basically all extern(C) functions should be have shared parameters and return types, because variables in C are not thread-local unless you jump through a bunch of hoops, using stuff that's not actually part of the language.

As it stands, we don't bother marking types on extern(C) functions with shared, and it's up to the programmer to understand what they're getting into and making sure that that they don't violate the thread-local guarantees that the D compiler provides for D code. If we then decide to start marking extern(C) functions with shared, the ones that most need to be marked are the ones that aren't thread-safe, not the ones that are.

Deciding to use shared on an extern(C) function because it's thread-safe, makes no sense at all. If it's thread-safe, then you can get way with passing thread-local data, whereas if it's not, then the data needs to be treated as shared by the D code - whether that's by marking up the extern(C) function with shared or by having the D code just be aware of the semantics and and dealing with them accordingly.

So, if we want to start marking extern(C) functions with shared, then we should be marking the ones that clearly operate on data that is intended to be shared across threads as shared, and whether the function is thread-safe is pretty much irrelevant.

@@ -192,7 +230,20 @@ class Mutex :
* Returns:
* true if the lock was acquired and false if not.
Copy link
Member

Choose a reason for hiding this comment

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

You need to fix it so that tryLock's documentation no longer says that it throws a SyncError. And actually, that bit of documentation is wrong even without your changes, since it just calls a C function and doesn't throw anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed.

@@ -163,7 +167,7 @@ class Mutex :
lock_nothrow();
}

// undocumented function for internal use
/// ditto
final void lock_nothrow(this Q)() nothrow @trusted @nogc
Copy link
Member

Choose a reason for hiding this comment

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

Are you documenting these because the normal versions can't be made nothrow and @nogc without breaking code? You can overload on attributes, but that still breaks code since you get an error about overriding one of the functions shadowing the others. I can see someone overriding lock and unlock for debugging purposes, but it seems really dumb for the version of the function that you would ideally call to not be lock or unlock... Also, the _nothrow function names don't actually follow the official naming convention, though lockNothrow does seem a bit ugly. As such, I'd be inclined to find a better name for them, but I don't know what it would be. Something like lockMutex would follow the naming convention and doesn't seem as ugly, but it seems redundant, and it doesn't make the difference between it and lock obvious.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since those methods (lock, unlock, tryLock) are virtual, we can't add attributes to them without breaking code. @MartinNowak attempted to enforce nothrow back then when 2.067 was to be released, but that broke vibe.d and had to be reverted. You can read more details about why vibe.d needed to throw from the locking primitives in this thread (in short - in order to implement interruptable tasks with proper resource cleanup).

I'm not 100% sure if we should document those functions, but that's what we should actually try to advertise as it is more friendly to code that tries to apply attributes where possible. I'm also not fan of the names, but considering that they're already public (albeit undocumented) I think we're kind of stuck with them. The only alternative names that I can think of are acquire and release, though tryAcquire sound bad to me.

Perhaps we can add lockExcept / unlockExcept virtual functions and deprecate the throwing methods, but I prefer not to do that in this change.

@PetarKirov PetarKirov force-pushed the add-attributes-to-core-sync-mutex branch 2 times, most recently from b0fe0f1 to 2ceb5cc Compare January 14, 2017 00:51
@PetarKirov
Copy link
Member Author

Thanks for the review @jmdavis. I've added unittests and I've addressed all your points.
Let me know if there's anything left to do.

@PetarKirov
Copy link
Member Author

Ping @andralex @WalterBright can you help me move forward with this PR? I need it continue working on adding more attributes in druntime and phobos.

@andralex
Copy link
Member

ping @jmdavis

@andralex
Copy link
Member

Nice work!

@@ -18,11 +18,11 @@ module core.sync.mutex;

public import core.sync.exception;

version( Windows )
version (Windows)
Copy link
Member

Choose a reason for hiding this comment

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

noice

}

/// ditto
final void unlock_nothrow(this Q)() nothrow @trusted @nogc
Copy link
Member

Choose a reason for hiding this comment

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

unlockNothrow? (though truth be told the underscore convention is also used)

Copy link
Member Author

Choose a reason for hiding this comment

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

unlock_nothrow was introduced in 2.067, so there's not much that I can do now.

@@ -499,8 +499,11 @@ int pthread_key_delete(pthread_key_t);
int pthread_mutex_destroy(pthread_mutex_t*);
int pthread_mutex_init(pthread_mutex_t*, pthread_mutexattr_t*) @trusted;
int pthread_mutex_lock(pthread_mutex_t*);
int pthread_mutex_lock(shared(pthread_mutex_t)*);
Copy link
Member

Choose a reason for hiding this comment

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

yah shared is good here

@andralex
Copy link
Member

OK let's move this forward once @jmdavis takes a second look

@PetarKirov
Copy link
Member Author

Thanks for the review @andralex!

@PetarKirov
Copy link
Member Author

Ping @jmdavis is there anything left for me to do?

Copy link
Member

@jmdavis jmdavis left a comment

Choose a reason for hiding this comment

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

I think that the one bit with pthread_mutexattr_destroy should be fixed (which I missed on my previous review), and then it's ready to go. I'm not entirely enthused with the idea of marking some of the pthread functions with shared, but Andrei seems to be in favor, so it's okay, I guess.


if( pthread_mutexattr_settype( &attr, PTHREAD_MUTEX_RECURSIVE ) )
throw new SyncError( "Unable to initialize mutex" );
scope (exit) pthread_mutexattr_destroy(&attr);
Copy link
Member

Choose a reason for hiding this comment

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

It seems inconsistent that the result of pthread_mutexattr_destroy is not checked. It probably should be. I'm not sure that that's really blocker though, since it hasn't been being checked. It would just be an improvement that was missed with this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I'll fix this later today.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed & rebased

@PetarKirov PetarKirov force-pushed the add-attributes-to-core-sync-mutex branch from 2ceb5cc to 90b1db6 Compare January 24, 2017 20:17
@PetarKirov
Copy link
Member Author

PetarKirov commented Jan 25, 2017

@jmdavis Merge pls?

Even though I added three unittests codecov is complaining because the aborts are not tested and to be honest, I have no intention of doing so, because that's code that shouldn't really be executed.

@@ -43,6 +42,9 @@ else

/**
* This class represents a general purpose, recursive mutex.
*
* Implemented using pthread_mutex on Posix and CRITICAL_SECTION
Copy link
Member

Choose a reason for hiding this comment

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

Wrap pthread_mutex and CRITICAL_SECTION in backquotes

{
InitializeCriticalSection( &m_hndl );
InitializeCriticalSection(cast(CRITICAL_SECTION*)&m_hndl);
Copy link
Member

Choose a reason for hiding this comment

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

space after cast(...) cc @wilzbach

throw new SyncError( "Unable to initialize mutex" );
scope(exit) pthread_mutexattr_destroy( &attr );
!pthread_mutexattr_init(&attr) ||
abort("Error: pthread_mutexattr_init failed!");
Copy link
Member

Choose a reason for hiding this comment

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

No exclamation marks please, just use a period.

if( pthread_mutexattr_settype( &attr, PTHREAD_MUTEX_RECURSIVE ) )
throw new SyncError( "Unable to initialize mutex" );
scope (exit) !pthread_mutexattr_destroy(&attr) ||
abort("Error: pthread_mutexattr_destroy failed!");
Copy link
Member

Choose a reason for hiding this comment

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

ditto

if( pthread_mutex_init( &m_hndl, &attr ) )
throw new SyncError( "Unable to initialize mutex" );
!pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE) ||
abort("Error: pthread_mutexattr_settype failed!");
Copy link
Member

Choose a reason for hiding this comment

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

ditto

abort("Error: pthread_mutexattr_settype failed!");

!pthread_mutex_init(cast(pthread_mutex_t*)&m_hndl, &attr) ||
abort("Error: pthread_mutex_init failed!");
Copy link
Member

Choose a reason for hiding this comment

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

ditto

m_proxy.link = this;
this.__monitor = &m_proxy;
this.__monitor = cast(void*)&m_proxy;
Copy link
Member

Choose a reason for hiding this comment

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

space

@@ -89,28 +106,43 @@ class Mutex :
* In:
* o must not already have a monitor.
*/
this( Object o ) nothrow @trusted
this(Object o) @trusted nothrow @nogc
Copy link
Member

Choose a reason for hiding this comment

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

o is awkward

Copy link
Member Author

@PetarKirov PetarKirov Jan 25, 2017

Choose a reason for hiding this comment

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

What about obj? Any better suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

obj is fine - pretty much anything but o, O, I, and l :)

assert( !rc, "Unable to destroy mutex" );
import core.internal.abort : abort;
!pthread_mutex_destroy(&m_hndl) ||
abort("Unable to destroy mutex");
Copy link
Member

Choose a reason for hiding this comment

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

period at end

@andralex
Copy link
Member

To be perfectly correct, basically all extern(C) functions should be have shared parameters and return types

That's incorrect. Most extern(C) functions assume they are not called with racy unsynchronized parameters, and the fact that we can enforce that in D at the type system level is a plus, not a minus.

@PetarKirov
Copy link
Member Author

@andralex fixed & rebased.

@jmdavis
Copy link
Member

jmdavis commented Jan 25, 2017

To be perfectly correct, basically all extern(C) functions should be have shared parameters and return types

That's incorrect. Most extern(C) functions assume they are not called with racy unsynchronized parameters, and the fact that we can enforce that in D at the type system level is a plus, not a minus.

Yes and no. The C stuff isn't thread-local, and that means that technically, it's all shared, even if it's not marked as such. In effect, it's all __gshared. It's really shared, but we're lying about it so that it works with D code. So, if we were to mark it all up as shared, that would be completely correct, whereas marking them all as thread-local is a lie. It's just that it would be super annoying to use them if they were all marked with shared, and most_C functions can be called safely as if they were thread-local. So, the approach that we've been taking of not using shared on any of them works without being super annoying so long as the programmer is aware enough of what's going on to treat the functions that really aren't thread safe as if they were shared.

Adjusting that approach so that we mark extern(C) functions with shared when we know that they aren't thread-safe then makes it safer for the programmer, because they're then actually forced to deal with shared when we know that it matters. They still have to be careful with other extern(C) functions that aren't marked with shared and be aware of what those functions actually do, because it's quite possible that they aren't thread-safe, since they are actually operating on shared data, but marking the ones that are definitely not thread-safe as shared at least makes it clear for those functions and removes the need for the programmer to figure it out in their case. So, that approach does make sense. But it's still true that all of the other extern(C) functions are operating on shared data that's not properly marked as shared. We're just letting it slide, because the code is theoretically thread-safe in spite of that.

So, the most correct thing would be to marked the all as shared. It just wouldn't be the safest thing to do, because then everyone would just be casting left and right, whereas if we use shared selectively, they'll pay attention when it's there. Right now, since we're not using shared with extern(C), we're basically doing the equivalent of slapping @trusted on every extern(C) function except that it's for thread-safety. Selectively using shared on extern(C) functions is then then not slapping the @trusted of thread-safety on those functions. But thanks to how the type system treats extern(C) functions, by default, they're assumed to be thread-safe when they really should be assumed to not be thread-safe. Selectively using shared therefore makes sense, but it's like we're having to mark stuff with @system manually rather than the compiler catching it for us. Unfortunately, fixing that would break far to much code, so selectively using shared is probably the best that we can do at this point.

@andralex
Copy link
Member

Yes and no. The C stuff isn't thread-local, and that means that technically, it's all shared, even if it's not marked as such. In effect, it's all __gshared. It's really shared, but we're lying about it so that it works with D code.

This is wrong, almost backwards actually. The lengthy explanation that follows does not help either :). Consider printf(const char* format, ...); Can a shared format string be passed by C code into C code? Absolutely. Does printf implement any mechanism for working with format strings that are actually shared across threads? No. Would printf actually work well if the string were modified concurrently by another thread? Most assuredly not. It follows that the printf signature is not sufficient - there is an assumption embedded so deeply it's not even part of the documentation that the format string is supposed to be effectively thread-local for the duration of the call. (This is of course not the case for all C functions but for a good fraction thereof.) So the D signature of printf is paradoxically more precise because it clarifies the assumption that the format string (and all other parameters) are thread-local.

@jmdavis
Copy link
Member

jmdavis commented Jan 25, 2017

The issue is that unlike D code, the C code makes no guarantees about operating on thread-local data, because normally, everything in C is shared. So, any time that D is calling a C function, the programmer has to be aware of whether the function is actually thread-safe or not and program accordingly rather than having that be clear in the type-system like it is with D. So, if a programmer is sure that the extern(C) function is thread-safe, then not having it be marked as shared is perfectly fine, and then the type system _does _help them - but it's up to the programmer to figure that out and mark the function accordingly. printf can obviously be treated as thread-safe, but because the compiler doesn't verify thread-safety anywhere with extern(C) functions, somewhere in the process a D programmer needs to verify it.

Basically, because extern(C) provides no compiler guarantees about what's going on inside these functions, every time the programmer is not using shared on an extern(C) function, they're either saying that they're not worrying about shared and it's up to the caller to deal with it correctly (which, for better or worse, is what we've done historically), or they're promising that it's actually thread-safe. On the other hand, if they're marking it as shared, then they're saying that they think that it's not thread-safe. The safest default would be to just use shared for all extern(C) functions precisely for the reason that assuming that a function is @system unless it's been verified by the compiler or programmer to be otherwise makes sense. But instead, it's basically entirely up to the programmer to mark the function correctly, and no one uses shared on extern(C) functions. So, the verification is always left up to the caller, and there's no way that that's always done correctly.

So, moving to marking extern(C) functions with shared where they're not thread-safe would then at least remove the need for the caller to do that verification, but we're then left with the problem that the assumption is that an extern(C) function without shared is thread-safe when the reality of the matter is that it could simply be that the person who did the bindings didn't bother with shared (and basically no one has thus far - certainly not in druntime). Still, marking extern(C) functions with shared when we know that they're not thread-safe is likely the best course of action.

I still think that it would be safer though if the language treated extern(C) functions as implicitly shared and forced the programmer to mark them as thread-local, even if most C functions would then be marked that way, since the way it is easily leads to passing thread-local D stuff to C code that isn't thread-safe. But then we'd need a special attribute just for extern(C) functions, because that's the complete opposite of what the D code does, and unfortunately, human nature being what it is, programmers would probably just slap the @local or whatever on extern(C) left and right rather than actually verifying that the function is thread-safe, particularly since most C functions are in spite of the fact that C itself doesn't do anything with thread-local or guaranteeing thread-safety.

So, I guess we disagree on what the default really should be with regards to shared and extern(C), but at this point, I don't think that we disagree about when an extern(C) function should be marked as shared. It is effectively a change in policy in how we handle C bindings though, and all of the druntime C bindings should probably be examined and change accordingly (at minimum with a shared overload being added, and ideally, at some point, the thread-local overloads would be deprecated).

@andralex
Copy link
Member

andralex commented Jan 25, 2017

The issue is that unlike D code, the C code makes no guarantees about operating on thread-local data, because normally, everything in C is shared.

No. Normally most in C is NOT shared, it's just that that information is not represented in the type system. Typical C programs have carefully-managed portions of data that is actually shared and specific narrow APIs for it. Don't confuse de jure with de facto.

So, any time that D is calling a C function, the programmer has to be aware of whether the function is actually thread-safe or not and program accordingly rather than having that be clear in the type-system like it is with D.

Not if the D signature of the C binding is written correctly.

So, if a programmer is sure that the extern(C) function is thread-safe, then not having it be marked as shared is perfectly fine, and then the type system _does _help them - but it's up to the programmer to figure that out and mark the function accordingly.

Most definitely @shared is the wrong default to go by seeing as the vast majority of C functions already assume the data is not shared.

printf can obviously be treated as thread-safe, but because the compiler doesn't verify thread-safety anywhere with extern(C) functions, somewhere in the process a D programmer needs to verify it.

The onus is small - only for the handful of C functions that actually do perform synchronization. The situation doesn't warrant multiple walls of texts.

@dnadlinger
Copy link
Member

dnadlinger commented Jan 25, 2017

On the other hand, if they're marking it as shared, then they're saying that they think that it's not thread-safe.

That's completely backwards when it comes to the interface of a function. However ill-defined shared is currently, on a function parameter it means that the function is able to deal with shared data. Pretty much only synchronization primitives and operations on lock free data structures fall into this category, but the vast majority of parameters doesn't – as Andrei points out, printf certainly can't be expected to handle concurrent modifications to the format string.

@andralex
Copy link
Member

Auto-merge toggled on

@PetarKirov
Copy link
Member Author

Thanks, @andralex! Can you also have a look at #1726?

@andralex andralex merged commit 763343d into dlang:master Jan 26, 2017
@PetarKirov PetarKirov deleted the add-attributes-to-core-sync-mutex branch January 26, 2017 15:43
}

/// ditto
this() shared @trusted nothrow @nogc
Copy link
Member

Choose a reason for hiding this comment

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

@ZombineDev
Unfortunately this broke sub-classes, see Issue 17130 – [Reg 2.074] ambiguous implicit super call when inheriting core.sync.mutex.Mutex. Would be nicer to fix this in dmd though.

Copy link
Member Author

@PetarKirov PetarKirov Jan 31, 2017

Choose a reason for hiding this comment

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

This indeed looks like a compiler bug. If the base class has multiple default constructors, the compiler should synthesize multiple default constructors in the derived class. I.e. the base class and the derived class should have equivalent default constructor overload sets. (Assuming no user-defined default constructors in the derived class.)

Do you think that the front-end can be fixed in a relatively short-term period? If not, probably the best course of action would be to revert this PR, though I prefer not to.

Hopefully the project tester can prevent such issues from coming up next time. I wonder why it didn't run for the whole month my PR was up for review.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants