Conversation
…ss obvious choices
rieutord
approved these changes
Feb 6, 2024
Contributor
Author
|
I'd still want to deal with these two things before merging:
So don't hesitate to give your opinion on any choice I made or thing I said, we can only profit from different perspectives especially because I don't know ESTER very well PS: You can quote someone on GitHub with this syntax: > something I said it'll look like that:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivations
A good logging tool/system is very useful to both the programmer and the user
ESTER already had the shape of one but two macros was co-existing
ester_*andLOG*Also in several places of the code base
fprintf(stderr, ...)was used, bypassing the logging tools, probably because the programmer didn't know of its existence.Problems and ambiguous situations
ester_errwas used, but often the next line wasexit(1)whileexit(EXIT_FAILURE)was already included in the macro making it useless and showing the lack of knowledge around the macro.But more importantly this misusage of the macro raised doubts about the cases where
ester_errwas used but the function continued afterwards, code that will not be executed becauseester_errembed anexitcall, I noticed such ambiguous use in these files:atm_onelayer.cppphysics.cppThese cases were more difficult to dealt with when replacing old calls with new one because even if it was useless we knew that ESTER run well as it is:
ester_errwhich doesn't exit but risk the appearance of new bugs?ester_criticalkeeping the same behavior as beforeNew macros proposal
ester_debug:ester_info:ester_warn:+ indicate line and file in DEBUG
ester_err:ester_warnwith red "Error:"+ print stack in DEBUG
ester_critical:ester_errwith red "CRITICAL:"+ exit program or throw exception
Examples of how it looks (without the color):
Changes to the code
LOGInorLOGWwere usedester_err(the ones that obviously want to exit directly) have been replaced byester_criticalkeeping the same behavior// Useless because ester_critical already exiton uselessexit()calls orreturnthat won't ever be reachedsolver.cpp,atm_onelayer.cpp,physics.cpp,polytrope.cpp,eos_freeeos.cpp,eos_opal.cppI chose to replace byester_criticalkeeping the same behavior as before but each of these case could be debated to let the function return and above function read an error code and exit themselvesstar2d::initthe choice has been made to let the function return 1 and let the program exit viaester_criticalinstar2d.cpp:main--> see Future changesstar1d::initfew changes were made contrary tostar2d::inithence the need of a future homogenizationsolver.cpp, ...Roadmap
Logging refactor/update:
<log level>: (<optional function/method>) <msg>with color on the log level partester_errbyester_errorester_criticalgiven the case and format each logLOGEbyester_errand formatfprintf(stderr, ...byester_errorester_critical:stat_evol.cpp,mat_math.cpp,matrix_block_diag.cpp,matrix.cpp,RFK_solver.cpp,SDIRK_solver.cpp,solver_full.cpp,solver.cpp,star_map.cpp,star1d_class.cppester_criticalthat I tagged// Useless because ester_critical already exitester_errinstead ofester_criticaland let higher methods/functions exit withester_criticalproviding more global information to the userFuture changes
These things were not addressed by this PR but could be by future one in particular #18
star1d::init,star2d::init,read_config.cpp,parser.cpp,star1d.cpp,star2d.cpp:main, exit withester_criticalprintf("error ...")should be replaced asfprintf(stderr, ..)had been but this could be more difficult (to check each printf call), this could be done smoothly not necessarily in one timeester_*macros and not raw printf/fprintf functionsConclusion
Even if this PR won't go into ESTER code base (even if I think it should in one form or another) it allowed me to dig deeper in the ESTER code base and ready to work on other improvements.
If you agree I'd like you to carefully review this PR, commit by commit, I took care to title them clearly.
Also I'm working on more, and more interesting!, than just that but I'd like to build upon this one if possible.
PS: in my opinion this work will be useless if future development keep the same habit of using custom macros/functions or fprintf instead of using ESTER designed macros
ester_*introduced here, it's a change of developing habits which is required.In general ESTER development should aim more consistency throughout the code base and this PR is trying to go in this direction!