-
Notifications
You must be signed in to change notification settings - Fork 65
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
Addressing a number of miscellaneous changes from #660 #680
Conversation
uwuw
and tally
uwuw
, tally
, and overlap_check
(I disabled notification on this.) Please ping me when this needs review :) |
uwuw
, tally
, and overlap_check
This should be ready for review @bam241 - not sure why tests aren't running? |
this happends some times... |
I manually restarted the failed build, this allowed GitHub to pick it up |
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.
Just one question, but LGTM !
thank you @gonuke
src/overlap_check/ProgressBar.hpp
Outdated
~ProgressBar(); | ||
~ProgressBar() { | ||
if (need_final_newline) { | ||
std::cout << "\n"; |
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.
maybe there we would want a endl
to flush the buffer ?
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.
👍
\n
should be the same as std::endl
, but endl
may be safer across platforms.
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.
I thought endl
would force the flush, where \n
would not ?
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.
LGTM!
astyle is failing, but not on my machine. I'm using v3.0.1... what to do??? :( |
we could move to I'll take a look and PR against your branch |
It's just one file.... |
astyle on test_KDENeighborhood.cpp
I have 3.1 |
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.
Just a few comments/questions. Looks really good overall to me.
Co-authored-by: Patrick Shriwise <[email protected]>
Co-authored-by: Patrick Shriwise <[email protected]>
Will merge at the end of the day if there are no further comments. |
Description
Implement code updates from #660 in
uwuw
tally
overlap_check
build_obb
misc/tests
Motivation and Context
These changes were proposed by the professional PullRequest review
Changes
These are largely syntax changes that don't impact the logic of the code.
Behavior
There is no change in behavior.
Other Information
Other relevant information to this pull request.