Skip to content

Conversation

@Pranavchiku
Copy link
Member

@Pranavchiku Pranavchiku commented Mar 3, 2024

Towards #3566. This PR adds a new node ArrayConstructor, adds asr verify check for ArrayConstant to only have *Constants, also checks that ArrayConstant cannot have another ArrayConstant.

| ArrayConstructor(expr* args, ttype type, expr? value, arraystorage storage_format)

Lot of lines can be removed from *_ArrayConstant_* that I'll prefer to do in subsequent PRs.

@Pranavchiku Pranavchiku added the asr ASR Related changes label Mar 3, 2024
@Pranavchiku Pranavchiku changed the title DEV : Introduce and implement ArrayConstructor` DEV : Introduce and implement ArrayConstructor Mar 3, 2024
@Pranavchiku Pranavchiku marked this pull request as ready for review March 3, 2024 19:24
@certik
Copy link
Contributor

certik commented Mar 3, 2024

Thanks! This requires careful review, I'll try to do it later today. It looks good though from first look.

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

I think that this is fine.

Do tests pass if in is_value_constant in asr_utils.h you change:

        } case ASR::exprType::ArrayConstant: {
            ASR::ArrayConstant_t* array_constant = ASR::down_cast<ASR::ArrayConstant_t>(a_value);
            for( size_t i = 0; i < array_constant->n_args; i++ ) {
                if( !ASRUtils::is_value_constant(array_constant->m_args[i]) &&
                    !ASRUtils::is_value_constant(ASRUtils::expr_value(array_constant->m_args[i])) ) {
                    return false;
                }
            }
            return true;
        }

to:

        } case ASR::exprType::ArrayConstant: {
            return true;
        }

?

This is important for asr verify in this PR to ensure that things are truly constant now.

@Pranavchiku
Copy link
Member Author

I’ll check this in a while. Thanks for reviewing.

@Pranavchiku
Copy link
Member Author

Everything works well locally, let's wait for CI tests.

@Pranavchiku Pranavchiku enabled auto-merge March 4, 2024 04:11
@Pranavchiku Pranavchiku disabled auto-merge March 4, 2024 04:11
@certik certik merged commit 64b25fa into lfortran:main Mar 4, 2024
gxyd pushed a commit to gxyd/lfortran that referenced this pull request Mar 5, 2024
DEV : Introduce and implement `ArrayConstructor`

Former-commit-id: 64b25fa
gxyd pushed a commit to gxyd/lfortran that referenced this pull request Mar 5, 2024
DEV : Introduce and implement `ArrayConstructor`
certik added a commit to certik/lfortran that referenced this pull request Mar 5, 2024
DEV : Introduce and implement `ArrayConstructor`
gxyd pushed a commit to gxyd/lfortran that referenced this pull request Mar 5, 2024
DEV : Introduce and implement `ArrayConstructor`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

asr ASR Related changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants