#722 Adding Host Name in Reporting #733
Conversation
src/reporter.cc
Outdated
| const char *BenchmarkReporter::Context::executable_name; | ||
|
|
||
| BenchmarkReporter::Context::Context() : cpu_info(CPUInfo::Get()) {} | ||
| BenchmarkReporter::Context::Context() : cpu_info(CPUInfo::Get()), sys_info(SystemInformation::Get()) {} |
src/reporter.cc
Outdated
| Out << LocalDateTimeString() << "\n"; | ||
|
|
||
| const SystemInformation &sys_info = context.sys_info; | ||
| Out << "System Name "<< sys_info.name << "\n"; |
There was a problem hiding this comment.
Is this printed only for the json dumper, or for all of them?
Put differently, i don't think it should be in the console output, it is getting rather crowded already.
There was a problem hiding this comment.
Removed from console and added only to JSON
src/sysinfo.cc
Outdated
| #endif | ||
| return str; | ||
| #else | ||
| const unsigned COUNT = 64; |
There was a problem hiding this comment.
HOST_NAME_MAX was not present on OSx, added local initialization.
The Job on Travis failed in config, can you please have a look at it?
src/sysinfo.cc
Outdated
| std::string GetSystemName() { | ||
| #if defined(BENCHMARK_OS_WINDOWS) | ||
| std::string str; | ||
| const unsigned COUNT = 32767; |
There was a problem hiding this comment.
Again, why invent a magic number.
MAX_COMPUTERNAME_LENGTH+1 ?
And that being said, isn't that a rather too big to have as an array on stack?
Maybe use resized std::string?
test/reporter_output_test.cc
Outdated
| AddCases(TC_ConsoleErr, | ||
| { | ||
| {"%int[-/]%int[-/]%int %int:%int:%int$", MR_Default}, | ||
| {"System Name", MR_Next}, |
There was a problem hiding this comment.
What is "system name"?
How is this different from "host name"?
If it is exactly the "host name", just use that term?
|
❌ Build benchmark 1584 failed (commit f70f5b39af by @jatinx) |
|
❌ Build benchmark 1585 failed (commit 7f7bb81561 by @jatinx) |
|
✅ Build benchmark 1586 completed (commit 7e957a81be by @jatinx) |
|
✅ Build benchmark 1587 completed (commit ffd0fa611f by @jatinx) |
|
✅ Build benchmark 1589 completed (commit 7fda14a749 by @jatinx) |
|
@LebedevRI did the asked changes, can you please have a look at it? |
|
@LebedevRI its been a week, is something wrong with my design, would like some comments if thats the case. |
src/sysinfo.cc
Outdated
| str = hostname; | ||
| #else | ||
| std::wstring wStr = hostname; | ||
| str = std::string(wStr.begin(), wStr.end()); |
There was a problem hiding this comment.
Does this work? Is this already done elsewhere in the library?
wchar is bigger than char, what makes sure that this isn't lossy?
There was a problem hiding this comment.
Using Standard wstring_convert to fix this
src/sysinfo.cc
Outdated
| #ifdef BENCHMARK_OS_MACOSX //Mac Doesnt have a copy for Host Name in it | ||
| #define HOST_NAME_MAX 64 | ||
| #endif | ||
| const unsigned COUNT = HOST_NAME_MAX; |
There was a problem hiding this comment.
Can't you just use HOST_NAME_MAX directly, why do you need COUNT variable?
There was a problem hiding this comment.
Removing unnecessary COUNT variable references, using macro directly
src/sysinfo.cc
Outdated
| int retVal = gethostname(hostname, COUNT); | ||
| if (retVal != 0) return std::string("Unable to Get Host Name"); | ||
| return std::string(hostname); | ||
| #endif |
There was a problem hiding this comment.
And others?
benchmark/src/internal_macros.h
Lines 39 to 73 in 439d6b1
PR with CLA signed