Skip to content
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

type_of metafunction should dealias the type #93

Merged
merged 12 commits into from
Oct 31, 2024

Conversation

changkhothuychung
Copy link

@changkhothuychung changkhothuychung commented Aug 27, 2024

Fix #91

Issue number of the reported bug or feature request: #

Describe your changes
Adding code to dealias the underlying type in case of using typedef-name

Testing performed
Updating current tests in libcxx to reflect the new expected behaviors.

Additional context

@changkhothuychung
Copy link
Author

changkhothuychung commented Aug 27, 2024

Hi @katzdm , can you help me take a look when you have time?

@changkhothuychung changkhothuychung changed the title initial attempt type_of metafunction should dealias the type Aug 27, 2024
@@ -2007,7 +2007,15 @@ bool type_of(APValue &Result, Sema &S, EvalFn Evaluator, DiagFn Diagnoser,

switch (RV.getReflectionKind()) {
case ReflectionKind::Null:
case ReflectionKind::Type:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I probably should have pointed to this earlier, but this is the spec for type_of:
https://brevzin.github.io/cpp_proposals/2996_reflection/d2996r6.html#pnum_210

type_of(^^int) fails to be a constant expression, so the existing behavior (i.e., diagnosing an error) is correct.

The cases that need modification are Object, Value, Declaration, BaseSpecifier, and DataMemberSpec.

(also for return_type_of below)

Ping me tomorrow if there's anything I can help with!

Comment on lines 2012 to 2014
bool UnwrapAlises = isa<UsingType>(currentType) ||
isa<TypedefType>(currentType) ||
isa<TemplateSpecializationType>(currentType);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity, what happens if we just pass /*UnwrapAliases=*/true unconditionally? Are we avoiding an undesirable behavior with this check, or is this an optimization? If the later, I'd prefer passing unconditional true.

(also for return_type_of below)

Copy link
Author

Choose a reason for hiding this comment

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

I updated to unconditional true and it looks fine to me. I updated some tests in libcxx to reflect the new behavior.

@changkhothuychung
Copy link
Author

I have this test fails locally - std/experimental/reflection/module-imports.sh.cpp

The reason is probably due to the arch of my laptop (mac M1). From the log - # error "We don't know how to get the definition of mbstate_t without <wchar.h> on your platform."

Can you help me run that test with my change to see if that runs ok?

@katzdm
Copy link
Collaborator

katzdm commented Sep 18, 2024

I have this test fails locally - std/experimental/reflection/module-imports.sh.cpp

The reason is probably due to the arch of my laptop (mac M1). From the log - # error "We don't know how to get the definition of mbstate_t without <wchar.h> on your platform."

Can you help me run that test with my change to see if that runs ok?

@changkhothuychung I believe I fixed this yesterday evening, while merging latest changes from upstream. Let me know if pulling from latest fixes the issue for you.

@changkhothuychung
Copy link
Author

@katzdm I pulled the latest from p2996, but it doesnt seem to fix the error above.

@katzdm katzdm merged commit cfdc5e2 into bloomberg:p2996 Oct 31, 2024
2 checks passed
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.

2 participants