customizable_test: revert the offsetof() build-related fixes #56
Conversation
They were made because the original version failed to compile with Clang 12 and 13, complaining about `offsetof()` being applied to a non-standard-layout type. The error is no longer emitted with Clang 14, so this is likely a compiler bug that was fixed, and there's no need for this dirty hack.
isaac-io
force-pushed
the
55-revert-the-offsetof-changes-to-customizable_test
branch
from
f0b9998 to
9eec87f
Compare
| #endif // ROCKSDB_LITE | ||
| RegisterOptions(&simple_, &simple_option_info); | ||
| } | ||
| SimpleConfigurable() { RegisterOptions(&simple_, &simple_option_info); } |
There was a problem hiding this comment.
Nit: I do not think this is the right format (RegisterOptions should be on the new line)
There was a problem hiding this comment.
./build_tools/format-diff.sh seems happy, and at least in my understanding of the Google C++ coding style, this is OK if it doesn't exceed the line length limit:
https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions
- The open curly brace is always on the end of the last line of the function declaration, not the start of the next line.
- The close curly brace is either on the last line by itself or on the same line as the open curly brace.
There was a problem hiding this comment.
It probably matches the google style but is not how most of the Rocks code is structured. The normal Rocks style appears to be more:
SimpleConfigurable {
RegisterOptions(....)
}
There was a problem hiding this comment.
In this case, that's how it was in the original code, and how it looks like in the current HEAD of the RocksDB repository:
|
@assaf-speedb this is a unit test only change, and the unit tests passed. I don't think that we need the functional QA cycle and performance test here to merge. Do you agree? |
|
@isaac-io - I agree here, and in general regarding test fixes. |
They were made because the original version failed to compile with Clang
12 and 13, complaining about
offsetof()being applied to a non-standard-layouttype. The error is no longer emitted with Clang 14, so this is likely a
compiler bug that was fixed, and there's no need for this dirty hack.