Skip to content

AK: Use Deducing this for OptionalBase #5447

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Hendiadyoin1
Copy link
Contributor

This is taken from and akin to
SerenityOS/serenity#25894


This greatly reduces the code duplication and is a bit more strict about forwarding

@Hendiadyoin1 Hendiadyoin1 requested a review from AtkinsSJ as a code owner July 14, 2025 22:25
@gmta
Copy link
Collaborator

gmta commented Jul 16, 2025

@Hendiadyoin1 CI is failing :-(

@Hendiadyoin1
Copy link
Contributor Author

Yeah, probably need to bump the ci compiler version

@ADKaster
Copy link
Member

If it doesn't build with Xcode 16.3 clang, we're gonna have to hold off on this. @BertalanD , wanna try this out on the latest Xcode 26 preview? Looks like beta 3 came out a week ago

@Hendiadyoin1
Copy link
Contributor Author

Ah whoops mainly forgot to remove a now superfluous thing
This should work on Xcode16.3 as that did bump the clang version to "17", which is actually LLVM 19
https://en.wikipedia.org/wiki/Xcode#cite_ref-208

@gmta gmta added the windows Related to the Windows platform; on PRs this triggers a Windows CI build label Jul 16, 2025
@BertalanD
Copy link
Member

Can confirm that Xcode 26 beta 3 (clang-1700.3.13.4) successfully builds this.

@gmta gmta requested a review from alimpfard July 17, 2025 08:52
@gmta gmta enabled auto-merge (rebase) July 21, 2025 13:35
auto-merge was automatically disabled July 21, 2025 20:34

Head branch was pushed to by a user without write access

@@ -32,10 +36,17 @@ struct ConditionallyResultType<false, T> {
using Type = T;
};

template<typename Self, typename T>
struct AddConstIfNeeded {
using Type = Conditional<IsConst<RemoveReference<Self>> && !IsConst<T>, AddConst<T>, T>;
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 what's the point of this? AddConst<T const> is just T const anyway
This name is really confusing, I think the intent is CopyConst<RemoveReference<Self>, T> but to keep T = U const as const if Self isn't const?

regardless the && !IsConst<T> is unnecessary, AddConst is idempotent for T const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good looked at AddConst and did not see any special handling for T const, good to know that it is idempotent anyway
And yes this is supposed to be CopyConstButDontRemoveItPls

Copy link
Contributor

@alimpfard alimpfard Jul 23, 2025

Choose a reason for hiding this comment

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

maybe a different name then, AddConstFrom<F, T> maybe?

template<typename From, typename To>
struct AddConstFrom {
    using Type = Conditional<IsConst<RemoveReference<From>>, T, AddConst<T>>;
};

the RemoveReference bit seems a bit odd to me, but probably okay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
windows Related to the Windows platform; on PRs this triggers a Windows CI build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants