Adding console output methods to some classes#872
Conversation
| // get the implicit complement, it's children will need updating | ||
| EntityHandle implicit_complement = 0; | ||
| rval = geom_tool()->get_implicit_complement(implicit_complement); | ||
| if (rval != MB_ENTITY_NOT_FOUND || rval != MB_SUCCESS) { |
|
Did you mean this to be extended from #855 (which we should get back to)? |
Ugh no. Accidentally combined some stashed changes I think. Fixing... |
|
I've merged #855, so maybe it will be easy to just rebase on that? |
|
If you rebase we can probably more forward on this @pshriwise |
gonuke
left a comment
There was a problem hiding this comment.
A few attempts to tidy... can't help but feel that there is some other external solution we can import
| return verbosity = val; | ||
| } | ||
|
|
||
| int get_verbosity(int val) { return verbosity; } |
There was a problem hiding this comment.
Why does this take an argument- seems confusing
| int set_verbosity(int val) { | ||
| int verbosity_max = 0; | ||
| int verbosity_min = 1; | ||
| if (val < verbosity_min || val > verbosity_max) | ||
| warning("Invalid verbosity value " + std::to_string(val) + | ||
| " will be set to nearest valid value."); | ||
| val = std::min(std::max(verbosity_min, val), verbosity_max); | ||
| return verbosity = val; |
There was a problem hiding this comment.
Unless my brain is missing something, this will always result in 0?
There was a problem hiding this comment.
Mm yeah mentally bit-flipped the initializer values at the beginning of this function.
| if (lvl > verbosity) return; | ||
| // otherwise, display message | ||
| if (newline) | ||
| std::cout << msg << "\n"; |
There was a problem hiding this comment.
| std::cout << msg << "\n"; | |
| std::cout << msg << std::endl; |
There was a problem hiding this comment.
I think it's preferred to use a newline literal these days. Using std::endl will force the output buffer to flush and is more likely to cause garbled messages in concurrent applications, so I thought I'd start taking us in that direction.
There was a problem hiding this comment.
Could do it all in one go in a separate changeset though.
There was a problem hiding this comment.
I thought it was preferred because it is platform independent (e.g. Windows carriage returns...). That's the only reason I suggested it.
Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>
|
Thanks for the review @gonuke! Not sure why the Housekeeping/Linux tests are failing at this point. Something about git permissions in the CI image. Made an attempt at addressing it, but no luck yet. I'll try to revisit it later if I have time. |
|
The failing the linux builds is something annoying that I was sure we had resolved many months ago. Not sure why it's coming up again. |
|
I made a quick scan for logging libraries and didn't find anything compelling - all a little heavier weight than I'd like. Do you think we could write a header-only small class that would do this and include it in the relevant places? |
|
We certainly could! I don't have the cycles to do so at the moment though |
Let me see how I might adapt what you have done in that way |
|
I think this was superseded by #876 |
| @@ -49,7 +49,7 @@ jobs: | |||
| - name: Checkout repository | |||
There was a problem hiding this comment.
I think this may fix the linux build test failure
| - name: Checkout repository | |
| - name: Add exception for dubious ownership | |
| run: git config --global --add safe.directory /__w/DAGMC/DAGMC | |
| - name: Checkout repository |
There was a problem hiding this comment.
Thanks, but I think this PR has been replaced by another ( #876 ), so will be close by @pshriwise soon.
|
Closing in favor of #876. @ahnaf-tahmid-chowdhury, if the changes you've suggested above apply in that PR as well would you mind adding them there also? |
This is a step toward improved console logging in some of our classes. As described in #856, DAGMC and other classes in MOAB always write some output, which can conflict with output in downstream applications and cause confusion.
message,warning, anderr_msgmethods have been added to thedagmcMetaDataandDagMCclasses. Warnings and errors will always be written to screen. Themessagemethod allows a verbosity level to be associated with the message. The message will either be written to screen or not depending on the verbosity level set on the class instance. I've specified two levels of verbosity for now, but I'm open to a more fine grain set of verbosity levels down the line.There's some duplicated code here, which could be improved. I'll look into abstracting these capabilieis into an object that both of the affected classes can inherit from perhaps.
Another thing I don't love about this is that a
std::stringstreamobject needs to be created for messages where conversion of non-char/string types occurs via the streaming operator. There are other libraries likefmtthat handle this more gracefully, but I'll leave that for an improvement in the future.