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

Add XCELIUM to existing IUS define checks. #1074

Merged
merged 1 commit into from
Aug 24, 2019

Conversation

cmarqu
Copy link
Contributor

@cmarqu cmarqu commented Aug 18, 2019

For some reason I missed to also check for the XCELIUM define when I introduced Makefile.xcelium, fix that.

@themperek
Copy link
Contributor

themperek commented Aug 19, 2019

Is not better to just keep one in the define (across the code)?

@cmarqu
Copy link
Contributor Author

cmarqu commented Aug 19, 2019

I thought about this but didn't like it so much - it then has some magic going on behind the scenes, instead of the current straightforward simname.upper() == DEFINE relation.
Also, I would expect that in the future with ghdl and Verilator getting incrementally more VPI/VHPI support, we may need to have more such switches based on both simulator names and also their versions, so the slightly more complex #if defined will not stand out much.

Of course, I'm open to change this if normalizing to a single define is the mainstream opinion.

@eric-wieser
Copy link
Member

The normalization could happen in the c preprocessor. Is there a name that covers both of these?

@cmarqu
Copy link
Contributor Author

cmarqu commented Aug 20, 2019

CADENCE would cover both.

@themperek
Copy link
Contributor

themperek commented Aug 20, 2019

For Questa we keep MODELSIM do not see why we cannot do the same here.

@cmarqu cmarqu force-pushed the add-XCELIUM-to-define-check branch from d9fb863 to e0c3300 Compare August 20, 2019 21:47
@cmarqu
Copy link
Contributor Author

cmarqu commented Aug 20, 2019

Alright, I now used the method of the existing ModelSim/Questa normalization.

@themperek themperek self-requested a review August 20, 2019 22:04
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Not sure this is the cleanest approach, but it's what we already do, and consistency is probably more important

@themperek themperek merged commit b365c01 into cocotb:master Aug 24, 2019
@imphil imphil added this to the 1.3 milestone Aug 26, 2019
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.

4 participants