Remove outputDebugString, replace getComputerName #2414
Conversation
Jenkins Build SummaryBuilt from this commit Built at 20180315 - 19:42:11 Test Results
|
| @@ -377,9 +375,7 @@ class StatsDCollectorImp | |||
| assert (! s.empty ()); | |||
| if (! buffers.empty () && (size + length) > max_packet_size) | |||
| { | |||
| #if BEAST_STATSDCOLLECTOR_TRACING_ENABLED | |||
There was a problem hiding this comment.
Was removing the BEAST_STATSDCOLLECTOR_TRACING_ENABLED here intentional?
There was a problem hiding this comment.
intentional -- my reasoning is that log() is already a no-op unless this is defined, so I didn't see the point if the two-level macro check. Feel free to suggest otherwise if you think the check should remain however.
|
I think one of the two travis failures was caused by the |
|
@scottschurr thanks for noticing that...that was actually a failure to git push properly on my part. That said, I'm not certain the failure(s) are entirely due to the vs project, so let's see what one more build does. Also, travis will be a bit happier with the next develop branch (pending). |
There was a problem hiding this comment.
This looks great.
Just for a bit of feature creep, I'll suggest removing getStackBacktrace() from SystemStats.h. Then you can nuke the entire file. I had to prove to myself it would be okay, so here's a commit if you'd like to cherry-pick: scottschurr@6ca3d27
src/ripple/beast/core/SystemStats.h
Outdated
| //============================================================================== | ||
| /** Returns the host-name of the computer. */ | ||
| std::string getComputerName(); | ||
|
|
||
| //============================================================================== | ||
| /** Returns a backtrace of the current call-stack. | ||
| The usefulness of the result will depend on the level of debug symbols |
There was a problem hiding this comment.
How would you feel about removing getStackBacktrace(), below, and removing this entire file. A grep suggests that getStackBacktrace() is unused and untested.
There was a problem hiding this comment.
slightly confusing, but getStackBacktrace() was already removed here: #2376 ...which is soon to be merged. There's probably a case to be made for merging to two PRs, but what I can do is just rebase and remove at that point.
There was a problem hiding this comment.
There's definitely a case for merging the two Prs so we can actually delete SystemStats.h. Otherwise a rebase has the potential for leaving the file hanging around and empty.
| } | ||
| //m_journal.trace << std::endl << ss.str (); | ||
| outputDebugString (ss.str ()); | ||
| std::cerr << std::endl; |
There was a problem hiding this comment.
This is not a point I care deeply about, but there is a minor behavioral change here. Separate writes to cerr (vs accumulating in a stringstream and dumping the whole stringstream at once) will somewhat increases the likelihood of garbled output if there are multiple threads writing to cerr.
Personally, I think this change is fine. C++11 only guarantees that cerr won't be corrupted. The individual characters from the threads can still end up interleaved (see discussion here: https://stackoverflow.com/questions/6374264/is-cout-synchronized-thread-safe).
There was a problem hiding this comment.
Thanks for pointing this out - certainly very subtle, and could cause some head-scratching.
In the absence of a guarantee that something like std::cerr << "a single string"; won't interleave, I don't think it's a huge issue; as you said, it's all about probabilities, and I'm not sure we can quantify how much more or less probable the garbling would be.
There was a problem hiding this comment.
std::cerr << std::endl; translates to std::cerr << '\n'; std::cerr.flush(); std::cerr.flush();. Prefer std::cerr << '\n';. You get the flush() implicitly with std::cerr.
Codecov Report
@@ Coverage Diff @@
## develop #2414 +/- ##
===========================================
- Coverage 70.32% 70.31% -0.01%
===========================================
Files 702 701 -1
Lines 53429 53426 -3
===========================================
- Hits 37573 37566 -7
- Misses 15856 15860 +4
Continue to review full report at Codecov.
|
src/ripple/app/misc/NetworkOPs.cpp
Outdated
| @@ -628,7 +628,7 @@ std::string | |||
| NetworkOPsImp::getHostId (bool forAdmin) | |||
| { | |||
| if (forAdmin) | |||
| return beast::getComputerName (); | |||
| return boost::asio::ip::host_name(); | |||
There was a problem hiding this comment.
Micro-optimization (although mostly aimed at ensuring the hostname doesn't change mid-execution), but should we do:
static std::string const hostname = boost::asio::ip::host_name();
if (forAdmin)
return hostname;
mellery451
added
the
Passed
|
In 1.0.0-b2 |
No description provided.