user properties: clean up API #90
Conversation
BusyJay
commented
Jul 6, 2017
BusyJay
commented
- reduce allocation
- collect lazily
- strict access mode
librocksdb_sys/crocksdb/c.cc
Outdated
There was a problem hiding this comment.
But make_unique is introduced in c++14?
BusyJay
force-pushed
the
busyjay/cleanup-properties
branch
from
e4dfd82 to
7cb2901
Compare
librocksdb_sys/crocksdb/c.cc
Outdated
There was a problem hiding this comment.
Do we have any style agreement on this?
There was a problem hiding this comment.
I don't think so. We should use librocksdb_sys/crocksdb/format-diff.sh to format c/c++ codes, which is adapted from rocksdb.
There was a problem hiding this comment.
but, I guess this may be a bug with the format tool 😓
There was a problem hiding this comment.
🤣 I know. We've borrowed the format-diff script, but leaves out .clang-format config.
There was a problem hiding this comment.
I myself use clang-format with google style as command line arguments, which is enough for this project.
There was a problem hiding this comment.
@andelf can you add the RocksDB clang format config?
There was a problem hiding this comment.
Copying the .clang-format from top dir of rocksdb solves
src/table_properties.rs
Outdated
src/table_properties.rs
Outdated
There was a problem hiding this comment.
Can we return a TableProperties<'a> and keep the inner pointer here?
It's a bit of magic for me to figure out how this work.
There was a problem hiding this comment.
I don't think it's suitable that TableProperties contains a lifetime. Maybe we can rename it to TablePropertiesRef. But I believe &'a TableProperties is more clear and rusty than TablePropertiesRef. Actually I don't invent the magic by myself, both CStr and str are implemented via similar logic.
There was a problem hiding this comment.
Seems this has already been discussed here, I think I'm OK with that.
librocksdb_sys/crocksdb/c.cc
Outdated
src/table_properties.rs
Outdated
There was a problem hiding this comment.
Maybe wrapping UserCollectedProperties struct is better.
Since we need fast index access for propties, not iterating over style access.
There was a problem hiding this comment.
I don't get it. UserCollectedProperties is a map, how do you access it via index?
There was a problem hiding this comment.
impl Index<&str> for it, wraps std::map operator[]
There was a problem hiding this comment.
Oh, I see. I thought you want to get (k, v) pair at specific position.
librocksdb_sys/crocksdb/c.cc
Outdated
There was a problem hiding this comment.
Maybe we can iterate the map to avoid constructing the string?
There was a problem hiding this comment.
It depends. I prefer using index access here.
There was a problem hiding this comment.
But it invokes copying the key here?
There was a problem hiding this comment.
Yes. But it compare less.
There was a problem hiding this comment.
Seems C++ 14 can find without constructing the string, can we use it?
There was a problem hiding this comment.
I'm not sure. I prefer stick to C++ 11 for now since rocksdb itself still just require C++ 11. It may be surprising if the wrapper requires a newer version of compiler.
src/rocksdb.rs
Outdated
There was a problem hiding this comment.
Yes. I'm waiting Travis to tell me the exact code he likes.
src/table_properties.rs
Outdated
There was a problem hiding this comment.
This should change to UserCollectedProperties?
|
LGTM |
|
Btw, a |
|
A |
BusyJay
force-pushed
the
busyjay/cleanup-properties
branch
from
b72feff to
7912996
Compare
|
LGTM |
|
@huachaohuang PTAL |
- reduce allocation - collect lazily - strict access mode
BusyJay
force-pushed
the
busyjay/cleanup-properties
branch
from
e5d79d6 to
fcef2d6
Compare
