-
Notifications
You must be signed in to change notification settings - Fork 75
Minor fixes #1420
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
Minor fixes #1420
Conversation
Comment in changelog.h was accidentally copied from an earlier PR
ilyamandel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks, @jeffriley !
I removed a repeated line from an earlier PR from changelog.h .
One optional suggestion: I think removing the use-mass-loss option should lead to an updated yaml.h and default yaml file. Yes, I realise the option is only deprecated, so nothing would go wrong with the old yaml file -- but I think of the yaml file as an "advertisement" of the available options, and if we don't want users to use deprecated options any more, we shouldn't advertise them.
|
I agree re yaml file @ilyamandel - completely slipped my mind. I'll push a new yaml file a bit later. |
|
P.S. One more thing: please remove --use-mass-loss from the list at the bottom of program-options-list-defaults.rst |
(i) Deprecated option
-use-mass-lossin favour of--mass-loss-prescription:Instead of using
--use-mass-lossor--use-mass-loss trueto enable mass loss, then specifying the mass loss prescription to be used with--mass-loss-prescription, mass loss can be enabled using--mass-loss-prescriptionwith any valid prescription (that is notzero), and disabled with--mass-loss-prescription zeroinstead ofuse-mass-loss false.Note: users might see a few odd error message if they use
--use-mass-lossand--mass-loss-prescriptionin the same command. Errors they may see are:"Commandline Options error: option '--mass-loss-prescription' cannot be specified more than once"
"Commandline Options error: the required argument for option '--mass-loss-prescription' is missing"
Both are because the commandline parsing code in COMPAS replaces deprecated options before handing the commandline over to boost to parse for options, so we may have duplicated
--mass-loss-prescription, and when we do that we may leave it without a parameter (because-use-mass-lossdoesn't need "true" (though it can have it).I could spend some time and energy (not insignificant) preventing those errors, but I don't think they'll cause too much confusion, and should be rarely seen.
(ii) Added compiler flag "-Wno-vla-cxx-extension" to "CXXFLAGS" in Makefile to suppress compiler extension warning
(iii) Fixed online docs for omissions in v03.22.02:
- fixed description for "--initial-mass-function" in "program-options-list-defaults.rst", and
- changed "--initial-mass-power" to "--initial-mass-function-power" in "program-options-list-defaults.rst"