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

Dyno resolve basic array indexing #26833

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

arifthpe
Copy link
Contributor

@arifthpe arifthpe commented Mar 4, 2025

Get basic array indexing with this working.

Components to doing this:

  • Adjust module code to work around nested procs in generic parents and nested proc variable capture in ChapelArray._validRankChangeArgs. Logged in https://github.com/Cray/chapel-private/issues/6154 to be reverted once support is added.
  • Fix a disambiguation bug in which promotion wouldn't be counted as an implicit conversion, so promoted candidates wouldn't be considered worse. This caused ambiguity when using this access on an array of strings, between array.this and promoted string.this.

Completes part of https://github.com/cray/chapel-private/issues/6107, and resolves https://github.com/Cray/chapel-private/issues/7196, which was blocked on array indexing.

Fixes the following spectests:

  • release/examples/spec/Arrays/Arrays/array-indexing
  • release/examples/spec/Arrays/Arrays/array-indexing-2
  • release/examples/spec/Arrays/Arrays/assoc-add-index
  • release/examples/spec/Arrays/Arrays/index-using-var-arg-tuple

Includes dyno testing for array indexing.

[reviewer info placeholder]

Testing:

  • dyno tests
  • paratest
  • fixed expected spectests
  • reproducer in backing issue resolves successfully

@arifthpe

This comment was marked as resolved.

@arifthpe arifthpe force-pushed the array-indexing branch 6 times, most recently from 5266ee6 to 6f081ef Compare March 6, 2025 19:27
@arifthpe arifthpe requested a review from DanilaFe March 6, 2025 19:27
@arifthpe arifthpe changed the title Apply nested proc workarounds for _validRankChangeArgs Dyno resolve basic array indexing Mar 6, 2025
@arifthpe
Copy link
Contributor Author

arifthpe commented Mar 6, 2025

@DanilaFe tests added, this is now actually ready for review

Comment on lines +331 to +342
if (call.name() == "this") {
if (call.actual(0).type().type() && call.actual(0).type().type()->isArrayType() &&
call.actual(0)
.type().type()
->toArrayType()
->eltType()
.type()
->isStringType()) {
gdbShouldBreakHere();
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

ready for review? Not so 😄

I see you're taking a page from my playbook here.

@@ -1254,6 +1262,12 @@ void DisambiguationCandidate::computeConversionInfo(Context* context, int numAct
}
}

if (canPass.passes() && canPass.promotes()) {
actualType = getPromotionType(context, fa1->actualType()).type();
gdbShouldBreakHere();
Copy link
Contributor

Choose a reason for hiding this comment

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

here's another.

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