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 "ghdl.flags" error in documentation #620

Merged
merged 1 commit into from
Feb 6, 2020
Merged

Fix "ghdl.flags" error in documentation #620

merged 1 commit into from
Feb 6, 2020

Conversation

LukasVik
Copy link
Contributor

@LukasVik LukasVik commented Feb 6, 2020

I believe this must be a copy-paste error. "ghdl.flags" is for compilation options, and is documented as such further up in the file. "ghdl.elab_flags" and "ghdl.sim_flags", which are for simulation options are documented below.

@eine
Copy link
Collaborator

eine commented Feb 6, 2020

Hi @LukasVik! I agree with fixing the location of that flag, however, I think that your interpretation might not be correct. I'd say that ghdl.flags is used both for compilation and simulation. Then, you can have extra elab flags and extra run flags. Hence, I think that the flag in section "Simulation options" should be moved down, together with other ghdl.* flags in the same section. But it should not be removed. Furthermore, it might be desirable to change the description so that "..passed to ghdl -a and ghdl --elab-run commands..." is said.

@eine
Copy link
Collaborator

eine commented Feb 6, 2020

Upon inspection of the sources (https://github.com/VUnit/vunit/blob/master/vunit/sim_if/ghdl.py) I was wrong and your interpretation is correct. I must have been mislead by some wip I have somewhere... So, I'm ok with with this PR.

@LarsAsplund, @kraigher, should we consider to rename ghdl.flags to, say, ghdl.analysis_flags in order to avoid this ambiguity. This would be breaking a change, so we might keep ghdl.flags as an alias.

@LukasVik
Copy link
Contributor Author

LukasVik commented Feb 6, 2020

Upon inspection of the sources (https://github.com/VUnit/vunit/blob/master/vunit/sim_if/ghdl.py) I was wrong and your interpretation is correct. I must have been mislead by some wip I have somewhere... So, I'm ok with with this PR.

Yeah it threw me for a loop as well. Had to inspect the code to find what was what.

should we consider to rename ghdl.flags to, say, ghdl.analysis_flags in order to avoid this ambiguity. This would be breaking a change, so we might keep ghdl.flags as an alias.

Makes sense. The current naming is confusing, as proven by this conversation.

@kraigher
Copy link
Collaborator

kraigher commented Feb 6, 2020

If we change the name the old name must still work but have a deprecation message for many releases before we remove it.

@kraigher kraigher merged commit 82ef224 into VUnit:master Feb 6, 2020
@LukasVik LukasVik deleted the ghdl_doc branch October 1, 2020 06:07
@eine eine added the Docs label Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants