Apply code review changes to make_watertight files #666
Conversation
…ging name of a struct.
| @@ -1,4 +1,3 @@ | |||
| message("") | |||
There was a problem hiding this comment.
Apologies in advance for commenting on such a small change.
The purpose of these lines is to break up the CMake output to make it easier to see what CMake output is coming from where. This message("") line appears in CMakeLists.txt files throughout the entire repo. If we don't want these lines anymore, then we should delete them from all the CMake files, not just this one. (I would argue we shouldn't delete them.)
There was a problem hiding this comment.
Ah, makes sense. Though I think there's something to their comment that it would be even better if these each contained a message indicating their respective directories.
https://github.com/svalinn/DAGMC/pull/660/files#r368664884
All the same, reverting this change now.
kkiesling
left a comment
kkiesling
left a comment
There was a problem hiding this comment.
Nice work! Just a few questions:
There are a lot of assert(moab::MB_SUCCESS == result) lines throughout (not necessarily ones that you've added). We discussed that result/rval should always be checked, but maybe there needs to be more than just an assertion. If the result is not a success, what error message shows up and is it helpful to users at all? Some places are good about having messages, while others still don't.
src/make_watertight/Arc.cpp
Outdated
| curve_i_verts, curve_j_verts); | ||
| if (moab::MB_SUCCESS != result) | ||
| std::cout << result << std::endl; | ||
| std::cout << "Failed to merge vertices with error: " << result << std::endl; |
There was a problem hiding this comment.
Should this be std::cerr?
There was a problem hiding this comment.
Mmm yeah probably. Updating now.
|
I had the same thought looking through this and it's been an issue in the |
kkiesling
left a comment
kkiesling
left a comment
There was a problem hiding this comment.
This PR looks good to me now. Thanks @pshriwise!
|
Any further comments or thoughts here @ljacobson64 @kkiesling? |
None from me. I approved the other day. |
bam241
left a comment
bam241
left a comment
There was a problem hiding this comment.
Thx @pshriwise !
Looking good.
I only have few minor comments.
|
Thanks @pshriwise |
5 participants
Applying the changes from the professional code review (#660) to the
make_watertightrelated files. The changes are as follows:Arc::create_loops_from_oriented_edges_fast)I'm not going to link each comment from the original PR here due to the sheer number of them, but I'm happy to answer any questions about the changes and hunt them down for reference as needed.