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

Implement is_structured_binding metafunction #58

Merged
merged 16 commits into from
Jun 23, 2024

Conversation

BaLiKfromUA
Copy link

@BaLiKfromUA BaLiKfromUA commented Jun 11, 2024

Implements #44

Describe your changes

Added implementation of is_structured_binding + adapted implementation of type_of and extract functions

Testing performed

Added tests for my changes to entity-classification.pass.cpp

 env LIT_FILTER=entity-classification.pass.cpp ninja check-cxx -C build-llvm/
....
Testing Time: 1.17s

Total Discovered Tests: 9692
  Excluded: 9691 (99.99%)
  Passed  :    1 (0.01%)

Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
@BaLiKfromUA
Copy link
Author

BaLiKfromUA commented Jun 11, 2024

I created this PR to receive feedback about my current progress while I am dealing with new implementation of #11 .

I tried to cover 3 main cases mentioned in the issue but will come back and add more tests later.

Plus, need to investigate needs for changes of related functions -- type_of, extract

result = isa<const VarDecl>(R.getReflectedDecl());
Decl *D = R.getReflectedDecl();
if (auto *BD = dyn_cast<BindingDecl>(D)) {
result = (BD->getHoldingVar() != nullptr);
Copy link
Author

Choose a reason for hiding this comment

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

Decided to go this way because of documentation:

  /// Get the variable (if any) that holds the value of evaluating the binding.
  /// Only present for user-defined bindings for tuple-like types.
  VarDecl *getHoldingVar() const;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Upon more careful reading of the Standard, I think I led you astray - we should not return true for is_variable in any of these cases.

I realized this while attempting to verify that getHoldingVar was the right API for this task, during which I came upon this code which implements tuple-like binding decomposition in clang. The synthesized variable RefVD corresponds to the variable

    S U_i r_i = initializer ;

mandated by [dcl.struct.bind]/4, but notice that it's never stored as a property of the BindingDecl B that's being constructed. Instead, the Sema::BuildDeclarationNameExpr() function is used to synthesize an expression referencing the variable, and that expression is set as the "binding" via B->setBinding(E).

Looking back at the Standard, this lines up:

Each v_i is the name of an lvalue of type T_i that refers to the object bound to r_i; the referenced type is T_i.

So even though a variable is synthesized for each binding in a tuple-like decomposition, I believe the binding itself is still not a variable, but rather a named expression referencing the synthesized variable.

Sorry for the mixup!

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for deep dive! I reverted changes to implementation but left existing tests

clang/lib/Sema/Metafunctions.cpp Outdated Show resolved Hide resolved
result = isa<const VarDecl>(R.getReflectedDecl());
Decl *D = R.getReflectedDecl();
if (auto *BD = dyn_cast<BindingDecl>(D)) {
result = (BD->getHoldingVar() != nullptr);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Upon more careful reading of the Standard, I think I led you astray - we should not return true for is_variable in any of these cases.

I realized this while attempting to verify that getHoldingVar was the right API for this task, during which I came upon this code which implements tuple-like binding decomposition in clang. The synthesized variable RefVD corresponds to the variable

    S U_i r_i = initializer ;

mandated by [dcl.struct.bind]/4, but notice that it's never stored as a property of the BindingDecl B that's being constructed. Instead, the Sema::BuildDeclarationNameExpr() function is used to synthesize an expression referencing the variable, and that expression is set as the "binding" via B->setBinding(E).

Looking back at the Standard, this lines up:

Each v_i is the name of an lvalue of type T_i that refers to the object bound to r_i; the referenced type is T_i.

So even though a variable is synthesized for each binding in a tuple-like decomposition, I believe the binding itself is still not a variable, but rather a named expression referencing the synthesized variable.

Sorry for the mixup!

Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
@BaLiKfromUA
Copy link
Author

BaLiKfromUA commented Jun 18, 2024

Thanks for the initial review! I will work on testing and adapting type_of and extract implementations.

So far, at least type_of implementation is definetely needs few small changes.

After my test&fixes, I'll request your review again + meanwhile will investigate which implemented functions might need some changes

…inding

Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
@BaLiKfromUA
Copy link
Author

BaLiKfromUA commented Jun 23, 2024

I think I am ready for next iteration of review. I slightly changed implementations of type_of and extract functions.

One remark about my changes: I am not sure about my changes to extract function because I didn't come up with all relevant test cases. My current tests are inspired by tests from to-and-from-values.pass.cpp.

constexpr auto p = std::pair{1, 2};
auto& [x4, y4] = p;

static_assert(extract<int>(^x4) == x4);
static_assert(extract<int&>(^x4) == x4);
static_assert(&extract<int&>(^x4) == &x4);

static_assert(extract<int>(^y4) == y4);
static_assert(extract<int&>(^y4) == y4);
static_assert(&extract<int&>(^y4) == &y4);

@BaLiKfromUA BaLiKfromUA requested a review from katzdm June 23, 2024 10:48
@katzdm
Copy link
Collaborator

katzdm commented Jun 23, 2024

I think I am ready for next iteration of review. I slightly changed implementations of type_of and extract functions.

One remark about my changes: I am not sure about my changes to extract function because I didn't come up with all relevant test cases. My current tests are inspired by tests from to-and-from-values.pass.cpp.

constexpr auto p = std::pair{1, 2};
auto& [x4, y4] = p;

static_assert(extract<int>(^x4) == x4);
static_assert(extract<int&>(^x4) == x4);
static_assert(&extract<int&>(^x4) == &x4);

static_assert(extract<int>(^y4) == y4);
static_assert(extract<int&>(^y4) == y4);
static_assert(&extract<int&>(^y4) == &y4);

Valentyn, I think these look great! Note that it hasn't been decided yet whether to support extract or type_of on structured bindings for P2996 (though the current wording for type_of seems like it would support it). Let's keep this for now though. Your changes will be on public godbolt tomorrow, so please feel free to poke and prod the APIs; let me know if anything looks wrong or crashes.

Thanks so much for the contribution!!

@katzdm katzdm merged commit 5157802 into bloomberg:p2996 Jun 23, 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