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

Fix a clash between CONTIGUOUS and SAVE attribute in flang (issue #673) #968

Merged
merged 1 commit into from
Feb 5, 2021

Conversation

pawosm-arm
Copy link
Collaborator

This simple patch adds CONTIGUOUS attribute to the list of attributes allowed to use along with the SAVE attribute.

Fixes a problem signaled in #673.

…ng-compiler#673)

This simple patch adds CONTIGUOUS attribute to the list of attributes
allowed to use along with the SAVE attribute.

Signed-off-by: Paul Osmialowski <pawel.osmialowski@arm.com>
Copy link
Collaborator

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

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

LGTM.

@bryanpkc
Copy link
Collaborator

@pawosm-arm This doesn't address the ordering issue, right? I didn't look in detail, but it seems that CONTIGUOUS would not conflict with any other attribute if it is specified later than the other ones?

How about the conflict between CONTIGUOUS and ASYNCHRONOUS, also mentioned in #673?

@pawosm-arm
Copy link
Collaborator Author

This doesn't address the ordering issue, right?

By ordering you meant the order those attributes are given? The funny thing (I've also mentioned in the bug report) is that SAVE, CONTIGUOUS compiled fine, while the other way round it didn't. With this patch, both orders are accepted.

I didn't look in detail, but it seems that CONTIGUOUS would not conflict with any other attribute if it is specified later than the other ones?

Indeed.

How about the conflict between CONTIGUOUS and ASYNCHRONOUS, also mentioned in #673?

I've looked into this just briefly, and it didn't seem to look as obvious as with CONTIGUOUS. Also, I don't have any real life code replicating that, while simultaneous use of CONTIGUOUS and SAVE has appeared recently in cp2k project's git master.

module contig
implicit none
integer, dimension(:), pointer, contiguous, save :: ptr
end module
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test case should probably be moved into test/sema/ I guess? Consider naming it contiguous.f90.

Copy link
Collaborator Author

@pawosm-arm pawosm-arm Jan 20, 2021

Choose a reason for hiding this comment

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

I don't think such a detailed division of test cases would give any additional value to the classic flang, also in the past I've been placing more test cases like this one in this same directory. And adding test case to this directory does not require modifying or adding any other files.

I've deliberately avoided naming this test case 'contiguous' to prevent name clash with one of the f2008 keywords.

@bryanpkc bryanpkc self-requested a review February 3, 2021 02:28
Copy link
Collaborator

@bryanpkc bryanpkc left a comment

Choose a reason for hiding this comment

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

This patch looks okay to me, but it would probably be useful to open an issue to remind ourselves of the inconsistent handling of CONTIGUOUS in the presence of other attributes..

@pawosm-arm pawosm-arm merged commit b225fa6 into flang-compiler:master Feb 5, 2021
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.

3 participants