Skip to content

Conversation

@ubaidsk
Copy link
Collaborator

@ubaidsk ubaidsk commented Aug 28, 2023

@ubaidsk ubaidsk requested a review from certik August 28, 2023 19:27
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 this looks great.

I found a minor issue in the way the dimensions are printed. This can be fixed in subsequent PRs.

--> tests/errors/arrays_05.py:6:5
|
6 | x: i16[5, 4] = empty([5, 3], dtype=int16)
| ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^ type mismatch ('i16[5][4]' and 'i16[5][3]')
Copy link
Contributor

Choose a reason for hiding this comment

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

I would write it as i16[5, 4] as in the source code, not i16[5][4].

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you also suggest to include the start value of the dimension? For example i16[0:5, 0:4]? I guess it is only helpful for the developers. For users it might confuse as python does not have lower_bounds. (Although it could be helpful in the case for LFortran if we reuse this printing mechanism/function there.)

We can also include the start value of the dimension later (when we have more experience with error messages or user feedback).

Copy link
Contributor

Choose a reason for hiding this comment

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

For Python I would print it as just i16[5, 4]. For LFortran we use a different method to print the type, and there indeed we have an option to print the dimension in the usual Fortran notation --- depending on the context, sometimes the lower dimension matters, sometimes it doesn't.

@certik certik merged commit c1e8486 into lcompilers:main Aug 28, 2023
@ubaidsk ubaidsk deleted the test_for_mismatch_in_empty_dtype branch August 28, 2023 22:17
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