Provides generic version of LifetimeEntryManager
#6212
Conversation
|
I wrote a rough benchmark, and it seems that the generic version does achieve the same effect while having higher performance. Of course this test is very simple, if anyone has better ideas, a more comprehensive test is also welcome. |
|
Performance is not a factor here. The "gains" above are negligible at best, if not straight up just measurement error. |
|
Another question is whether we need to replace |
|
This PR should include a preview of what the required game-side changes are after merging this. |
Yeah, just making sure I don't run into new performance issues.
In theory, I provide a non-generic version, so existing code doesn't need any modifications and still works as usual. In other words, it is feasible for other content to remain unchanged after the PR is merged. And if modification is needed, for classes such as - private readonly LifetimeEntryManager manager = new();
+ private readonly LifetimeEntryManager<DrawableLifetimeEntry> manager = new();
...
- private class DrawableLifetimeEntry : LifetimeEntry, IDisposable {
+ private class DrawableLifetimeEntry : LifetimeEntry<DrawableLifetimeEntry>, IDisposable {
...
}
...and remove all redundant type conversions. The process involving PooledDrawableWithLifetimeContainer, another layer of generics, is generally similar, except: public abstract partial class PooledDrawableWithLifetimeContainer<TEntry, TDrawable> : CompositeDrawable
- where TEntry : LifetimeEntry
+ where TEntry : LifetimeEntry<TEntry>
where TDrawable : Drawable {
...
} |
|
I would expect to see a preview of how converting the game-side usages to use the generics would look like, to evaluate if this implementation is the code quality improvement it claims to be. API surface changes must always be judged by the usage code. |
|
I don't quite understand how I should provide a preview, do I need to open another PR in the repo of osu!? |
|
A link to an un-PR'd branch or a patch would also do. |
|
I tried to modify Finding other classes that need to be modified (including those in o!f and osu!) and modifying them will take me a lot of time, so it may take a while before I propose a proper complete PR. Or I could try submitting a series of PRs to modify these classes one by one. |
| /// <typeparam name="TDerived">The implemented class itself. Used to provide derived category information for base categories using the Curiously Recurring Template Pattern(CRTP).</typeparam> | ||
| public abstract class LifetimeEntry<TDerived> where TDerived : LifetimeEntry<TDerived> |
There was a problem hiding this comment.
Nooooooooooooooooooooooooope. Nope nope nope nope. I'm not having anything that the cpp people invented. Especially not with a name like "curiously recurring". I don't even follow what this is supposed to be doing.
I will not accept this sort of arcane construction here. If this makes-or-breaks the entire series then it might as well be closed right now.
There was a problem hiding this comment.
I will not accept this sort of arcane construction here.
Sadly, this is necessary. Manager needs to use this type argument to get the actual type of the field.
I even thought this was a fairly common way of writing modern C#. I just want to clarify here what the real name of this mode is.😕
There was a problem hiding this comment.
I'm honestly having a difficult time accepting that linked article if it's giving IEquatable<T> as an example of this. IEquatable<T> is nowhere near as difficult to parse! It's not doing this IEquatable<T> where T : IEquatable<T> brain twister thing in any class signature. Its methods accept T.
There was a problem hiding this comment.
I just spent some time searching for something I myself learned about this many years ago. Luckily, I found it and I thought this article might be a good explanation.
| // Due to the type constraints of C#, we cannot declare `LifetimeEntry<T> where T = LifetimeEntry<T>` to limit the type of `this` to be `T`. But when used correctly, this will always be the case. | ||
| // aka. We can't stop anyone from writing code like this: | ||
| // ```csharp | ||
| // public class MyEntry : LifetimeEntry<MyEntry> { } | ||
| // LifetimeEntry<MyEntry> foo = new(); | ||
| // ``` | ||
| // We prevent users from inadvertently writing such code by declaring `LifetimeEntry<T>` as `abstract`, but we cannot prevent it completely. | ||
| // ```csharp | ||
| // public class NewEntry<T> : LifetimeEntry<T> where T : LifetimeEntry<T> { } | ||
| // NewEntry<MyEntry> a = new(); | ||
| // ``` | ||
| // Happily, however, the compiler correctly prevents code like this from compiling. This is also enough to deter users from writing incorrect code: | ||
| // ```csharp | ||
| // public class ErrorEntry : NewEntry<MyEntry> { } | ||
| // LifetimeEntryManager<NewEntry<MyEntry>> error1 = new(); // Compiler error. | ||
| // LifetimeEntryManager<ErrorEntry> error2 = new(); // Compiler error. | ||
| // ``` | ||
| RequestLifetimeUpdate?.Invoke((TDerived)this); |
There was a problem hiding this comment.
Case in point for the above diatribe. If API correctness of a method requires this sort of preamble to convince readers that the API surface is safe, the whole thing is overdone.
There was a problem hiding this comment.
Basically, it's the same thing, I'm just trying to illustrate why we still can't get away from typecasting. I simply think that a more detailed explanation will help myself or other users understand the design logic clearly.
These comments are not actually necessary, since I don't really need any additional constraints or commitments. These are written within the design scope of the compiler and I'm not trying to break anything.
I tried to clarify my thinking through comments, but now it seems like this is overkill?
There was a problem hiding this comment.
I'm not even sure what that is attempting to explain. Why is the first snippet "unwanted"? It looks like perfectly reasonable code. What is the second snippet supposed to be? What are NewEntry and MyEntry?
There was a problem hiding this comment.
I'm not even sure what that is attempting to explain. Why is the first snippet "unwanted"?
Because if the user writes code like that, the cast here will throw an exception.
LifetimeEntry<MyEntry>
// TDerived is "MyEntry" here.
// this.GetType() == LifetimeEntry
...
(MyEntry)this // throw an Exception: A base type cannot be cast to a derived type.Also notice that I declared this class as an abstract class to prevent users from doing something wrong like this.
There was a problem hiding this comment.
However, this subtle error will be exposed through the following compilation errors when writing. I just want to make this clear. After all, this recursive form of generics can be a bit confusing to understand how the compiler will handle it.
There was a problem hiding this comment.
What is the second snippet supposed to be? What are
NewEntryandMyEntry?
Assumptions about an extreme situation. Although I declared this class as abstract class. But like all abstract classes there are problems that may be encountered. You can't prevent users from implementing an abstract class without taking any meaningful action. This is basically like:
abstract class A {
// some thing here.
}
class B : A { } // class B is the same as A, but it's no longer abstract!This makes it possible for users to write illogical code.
Then in the third code block, I actually illustrate that even if this happens, the compiler will correctly prevent the user from doing this.
|
I understand that CRTP has caused confusion for someone, so please allow me to provide some clarification here: What is CRTP: CRTP is a trick but not a hack. It is used here to pass the Why CRTP: When implementing a generic public class Manager<T> where T : Entry {
protected T? item;
public void Foo(T entry) {
entry.Do += (sender) => {
this.item = sender;
}
}
}
public class Entry {
public event Action<Entry>? Do;
public void Bar() {
Do?.Invoke(this);
}
}Then obviously, public class MyEntry : Entry { }
...
MyEntry.Do.GetType() == typeof(Action<Entry>); // But we prefer it to be Action<MyEntry>Therefore, we need a type parameter to pass the type of the current class to the public class Manager<T> where T : EntryBase<T> {
protected T? item;
public void Foo(T entry) {
entry.Do += (sender) => {
this.item = sender;
};
}
}
public abstract class EntryBase<T> where T : EntryBase<T> {
public event Action<T>? Do;
public void Bar() {
Do?.Invoke((T)this);
}
}
public class Entry : EntryBase<Entry> { }Now, everything works fine, our Of course, we can notice that there is a type cast in Maybe you are uncomfortable with such lengthy comments, because it seems to imply that I am using comments to maintain the normal operation of a class. However, what I need to explain is that those comments are just for those who want to understand the design details. After all, I have to admit that many higher-level techniques are used internally in this class, and not everyone can easily understand them. The intent within the code. For users of this class, they only need to follow this rule: public class MyEntry : EntryBase<MyEntry> {
// something here
}
...
var manager = new Manager<MyEntry>;And the compiler will correctly block incorrect code. BTW, English is not my first language, so if there is any ambiguity in my explanation please point it out and I will rephrase it. |
|
But the comments above just now reminded me that maybe I should give |
|
Let me be very clear on this:
I do not find that an acceptable tradeoff at this time regardless of how many words of explanation are thrown at me. I am not interested in reviewing this series any further. If anyone else is they are free to do so but I will not participate in further discussions on this matter. |
|
Well, I respect your opinion. I regret that we did not reach a consensus.🥲 But in fact, users of these two classes don't even need to understand how they work. They only need to inherit from the generic class. So I don't think this invariably requires users to spend time mentally parsing it. Also, the non-generic version of these classes is still available without any change in functionality. |
|
As a fact, CRTP is a trick that is already used in .NET. For example: IAdditionOperators: public interface IAdditionOperators<TSelf,TOther,TResult> where TSelf : IAdditionOperators<TSelf,TOther,TResult> |
I noticed that the
LifetimeEntryManagerclass is not generic. This will cause me to have to cast repeatedly if I overload theLifetimeEntryclass:I thought maybe we should provide a generic version (LifetimeEntryManager) to avoid such excessive casts. In addition, I noticed that osu! also casts classes like this.
Ultimately, I settled on using CRTP as a trick to accomplish this. However, due to limitations of C# features, some functions may not be implemented perfectly. And for the sake of compatibility, I still provide a non-generic version to avoid some breaking-changes.