user properties: clean up API by BusyJay · Pull Request #90 · tikv/rust-rocksdb

BusyJay · 2017-07-06T06:44:03Z

reduce allocation
collect lazily
strict access mode

disksing · 2017-07-06T07:05:21Z

librocksdb_sys/crocksdb/c.cc

You can use make_unique().

But make_unique is introduced in c++14?

huachaohuang · 2017-07-06T09:46:10Z

librocksdb_sys/crocksdb/c.cc

Do we have any style agreement on this?

Clang-format with Google style

I don't think so. We should use librocksdb_sys/crocksdb/format-diff.sh to format c/c++ codes, which is adapted from rocksdb.

use the same style with RocksDB.

but, I guess this may be a bug with the format tool 😓

🤣 I know. We've borrowed the format-diff script, but leaves out .clang-format config.

I myself use clang-format with google style as command line arguments, which is enough for this project.

@andelf can you add the RocksDB clang format config?

Copying the .clang-format from top dir of rocksdb solves

huachaohuang · 2017-07-06T09:57:51Z

src/table_properties.rs

Don't need pub anymore :)

huachaohuang · 2017-07-06T10:04:54Z

src/table_properties.rs

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.

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.

Seems this has already been discussed here, I think I'm OK with that.

andelf · 2017-07-06T09:46:19Z

librocksdb_sys/crocksdb/c.cc

Better use c++ style cast

andelf · 2017-07-06T10:19:57Z

src/table_properties.rs

Maybe wrapping UserCollectedProperties struct is better.
Since we need fast index access for propties, not iterating over style access.

I don't get it. UserCollectedProperties is a map, how do you access it via index?

impl Index<&str> for it, wraps std::map operator[]

Oh, I see. I thought you want to get (k, v) pair at specific position.

huachaohuang · 2017-07-06T13:54:43Z

librocksdb_sys/crocksdb/c.cc

Maybe we can iterate the map to avoid constructing the string?

It depends. I prefer using index access here.

But it invokes copying the key here?

Yes. But it compare less.

Seems C++ 14 can find without constructing the string, can we use it?

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.

huachaohuang · 2017-07-06T13:58:30Z

src/rocksdb.rs

Seems not formatted here?

Yes. I'm waiting Travis to tell me the exact code he likes.

huachaohuang · 2017-07-06T14:03:45Z

src/table_properties.rs

This should change to UserCollectedProperties?

huachaohuang · 2017-07-06T15:43:00Z

LGTM

huachaohuang · 2017-07-07T04:29:50Z

Btw, a len() method can be helpful.

BusyJay · 2017-07-07T04:41:55Z

@zhangjinpeng1987 @disksing @andelf PTAL

huachaohuang · 2017-07-07T04:48:37Z

A len() for TablePropertiesCollection, thanks.

disksing · 2017-07-07T06:13:14Z

LGTM

BusyJay · 2017-07-07T06:21:07Z

@huachaohuang PTAL

huachaohuang

LGTM

- reduce allocation - collect lazily - strict access mode

zhangjinpeng87

LGTM

)" (#102) (#374)

BusyJay requested review from disksing, huachaohuang and zhangjinpeng87 July 6, 2017 06:44

disksing reviewed Jul 6, 2017

View reviewed changes

BusyJay force-pushed the busyjay/cleanup-properties branch from e4dfd82 to 7cb2901 Compare July 6, 2017 08:37

huachaohuang reviewed Jul 6, 2017

View reviewed changes

andelf reviewed Jul 6, 2017

View reviewed changes

huachaohuang reviewed Jul 6, 2017

View reviewed changes

BusyJay force-pushed the busyjay/cleanup-properties branch from b72feff to 7912996 Compare July 7, 2017 06:00

huachaohuang previously approved these changes Jul 7, 2017

View reviewed changes

user properties: clean up API (#90)

fcef2d6

- reduce allocation - collect lazily - strict access mode

BusyJay dismissed huachaohuang’s stale review via fcef2d6 July 7, 2017 09:34

BusyJay force-pushed the busyjay/cleanup-properties branch from e5d79d6 to fcef2d6 Compare July 7, 2017 09:34

zhangjinpeng87 approved these changes Jul 7, 2017

View reviewed changes

BusyJay merged commit a50c084 into master Jul 7, 2017

BusyJay deleted the busyjay/cleanup-properties branch July 7, 2017 15:28

yiwu-arbug mentioned this pull request Apr 30, 2019

Update to latest Titan #292

Closed

sre-bot added a commit that referenced this pull request Nov 4, 2019

titan: Revert "Relax warnings as errors so we can build on Clang 11 (#90

66e0f31

)" (#102) (#374)

Conversation

BusyJay commented Jul 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BusyJay Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huachaohuang commented Jul 6, 2017

Uh oh!

huachaohuang commented Jul 7, 2017

Uh oh!

BusyJay commented Jul 7, 2017

Uh oh!

huachaohuang commented Jul 7, 2017

Uh oh!

disksing commented Jul 7, 2017

Uh oh!

BusyJay commented Jul 7, 2017

Uh oh!

huachaohuang left a comment

Choose a reason for hiding this comment

Uh oh!

zhangjinpeng87 left a comment

Choose a reason for hiding this comment

BusyJay Jul 6, 2017 •

edited

Loading