Fix msvc build errors in block_cache_trace_analyzer.cc (#5746) #5838
Conversation
Use a dynamically allocated array for reuse_table, since msvc does not support stack-allocated arrays whose size is unknown at compile time. Add a couple of explicit casts to avoid numeric conversion warnings.
| @@ -650,7 +651,8 @@ void BlockCacheTraceAnalyzer::WriteCorrelationFeaturesToFile( | |||
| const std::map<std::string, Features>& label_features, | |||
| const std::map<std::string, Predictions>& label_predictions, | |||
| uint32_t max_number_of_values) const { | |||
| std::default_random_engine rand_engine(env_->NowMicros()); | |||
| std::default_random_engine rand_engine( | |||
There was a problem hiding this comment.
Indentation. Can you build_tools/format-diff.sh as described in https://github.com/facebook/rocksdb/wiki/RocksDB-Contribution-Guide?
| @@ -14,6 +14,7 @@ | |||
| #include <fstream> | |||
| #include <iomanip> | |||
| #include <iostream> | |||
| #include <memory> | |||
There was a problem hiding this comment.
<memory> is already included by including other headers.
There was a problem hiding this comment.
You're right, I originally thought that was why the xcode build was failing. But it turns out xcode clang doesn't like std::make_unique at all, at least with the compiler options that are being used here.
There was a problem hiding this comment.
Does this mean we can use #if defined() to enclose this include?
There was a problem hiding this comment.
I would just remove it altogether. If I had looked more closely I would have realized that std::unique_ptr is already used in several other places in this file, so including is clearly not necessary for this new use of it.
Alternatively you could keep it in; some people would argue that each file should explicitly include the headers for what it uses so that it is not dependent on other header files including them first. All of those headers will have include guards so the impact on compile time should be negligible.
| @@ -1209,11 +1211,14 @@ void BlockCacheTraceAnalyzer::WriteBlockReuseTimeline( | |||
| TraverseBlocks(block_callback); | |||
|
|
|||
| // A cell is the number of blocks accessed in a reuse window. | |||
| uint64_t reuse_table[reuse_vector_size][reuse_vector_size]; | |||
| const std::unique_ptr<uint64_t[]> reuse_table( | |||
There was a problem hiding this comment.
MSVC supports two-dimensional arrays, but does not support variable length arrays, i.e. dimension sizes need to be known at compile time. Variable length array is not part of recent versions of C++ standard.
There was a problem hiding this comment.
Will the following compile and run on MSVC?
#include <cstdio>
int main(int argc, char** argv) {
const size_t sz = argc;
int test_arr[sz];
fprintf(stdout, "size is %d\n", sizeof(test_arr));
return 0;
}
If not, this change is necessary.
It probably won't, according to your previous comment about compile-time.
There was a problem hiding this comment.
It does not compile: "error C2131: expression did not evaluate to a constant"
The compiler complains about [sz].
There was a problem hiding this comment.
Thanks @hliu18 for verifying this.
There was a problem hiding this comment.
I see this PR was imported but not checked in due to conflicts. Can someone help move this PR forward? I was going to fix a couple of build break on MSVC and noticed this.
There was a problem hiding this comment.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Fixed by #6081 |
Use a dynamically allocated array for reuse_table, since msvc does not
support stack-allocated arrays whose size is unknown at compile time.
Add a couple of explicit casts to avoid numeric conversion warnings.
N.B. I have not been able to test this change properly since I can't get the tests to run successfully using "make check" on Linux. Please let me know if there is a better way to do this.