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

Adding a test for generic packages on entities. #20

Merged
merged 1 commit into from
Mar 7, 2023

Conversation

bpadalino
Copy link
Contributor

This test is derived from GHDL issue #695 and seems to have different results for Questa, Riviera-PRO, ghdl, and nvc.

Questa wants the generic map to be a box (<>), and will simulate fine. Riviera-PRO will parse either actual map or the box, but simulation always fails with an elaboration error. nvc only wants the box, but then complains about the package being used incorrectly. ghdl was the source of the issue.

@bpadalino bpadalino mentioned this pull request Mar 7, 2023
@umarcor umarcor added Enhancement New feature or request VHDL-2008 labels Mar 7, 2023
@umarcor
Copy link
Member

umarcor commented Mar 7, 2023

@Paebbels @LarsAsplund @JimLewis shall we have an LRM Reading label in this repo?

@Paebbels
Copy link
Member

Paebbels commented Mar 7, 2023

If this is how we called it in GHDL and also in the VHDL-2019 repos, then we should have it here too.

@bpadalino
Copy link
Contributor Author

So just curious, what is the LRM reading required here? Section 6.5.5 of LRM-2008 seems pretty clear that:

interface_package_declaration ::= package identifier is new uninstantiated_package_name interface_package_generic_map_aspect

interface_package_generic_map_aspect ::= generic_map_aspect | generic map (<>) | generic map ( default )

It seems like it's just not a very widely used feature that has been tested, but it is legal.

Right?

@umarcor
Copy link
Member

umarcor commented Mar 7, 2023

From your description I understood that different tools were implementing this differently. So, LRM reading means we need to understand whether their interpretation/implementation is correct.

However, I now read it again and I think you meant that none of those tools implements the feature, all fail for various reasons. Is that the case?

@bpadalino
Copy link
Contributor Author

Yes, sorry for the confusion. None of the tools seem to conform to the standard. They all implement some subset.

The LRM is pretty clear, but the tools don't support it - hence the test. The test can probably be made into 3 different ones to test the 3 different scenarios - generic_map_aspect, generic map (<>), generic map (default).

@umarcor
Copy link
Member

umarcor commented Mar 7, 2023

@bpadalino thanks for clarifying. I removed the label.

Do you want to make the test into 3 before merging or shall I merge it and we can do that in an upcoming PR?

@bpadalino
Copy link
Contributor Author

Merge this and I can make an issue to split it into 3, then make a separate PR if that's OK.

@umarcor umarcor merged commit 3a1c817 into VHDL:main Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request VHDL-2008
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants