Implement the ZADD options

[manchurio]

This closes #1018

Has passed the unit test with go.

[PragmaTwice]

You can just run ./x.py format to format the source code rather than manual formatting.

[PragmaTwice]

Oh, I notice the cppcheck false alarm. I think you can change --std=c++11 to 17 here: https://github.com/apache/incubator-kvrocks/blob/unstable/x.py#L162

@manchurio

[manchurio]

You can just run ./x.py format to format the source code rather than manual formatting.

got it,thanks.

[manchurio]

Oh, I notice the cppcheck false alarm. I think you can change --std=c++11 to 17 here: https://github.com/apache/incubator-kvrocks/blob/unstable/x.py#L162

@manchurio

src/commands/redis_cmd.cc:2432:19: warning: Suspicious condition. The result of find() is an iterator, but it is not properly checked. [stlIfFind]
    if (auto it = options.find(option); it != options.end()) {
                  ^

It seems cppcheck wrongly reports @PragmaTwice

[PragmaTwice]

It is sad that cppcheck still fail and report a FALSE ALARM.

We will replace cppcheck by clang-tidy later, and I think you can now workaround it by auto iter = ...; if(iter != end()) ...

cc @git-hulk

[git-hulk]

@manchurio Need to use gofmt to format the test case

[tanruixiang]

If you only need to determine whether the key exists, you can use count directly, the iterator is redundant. Also doing this will not trigger an error from cppcheck. (By the way, C++20 has contains to do this thing. )

[PragmaTwice]

If you only need to determine whether the key exists, you can use count directly, the iterator is redundant. Also doing this will not trigger an error from cppcheck. (By the way, C++20 has contains to do this thing. )

Hi @tanruixiang,

Actually I like the original code more, since the iterator is used in the true-branch block to avoid a double search (the upper bound is actually not O(1) due to collision, and even O(1) DOES NOT imples no cost).

And I think this change does not improve readability, as the use of iterators is natural and easy to understand in c++. The false positives of cppcheck are entirely due to his own problems, and we should not modify the code logic just for that.

And in my opinion, I prefer just a comment rather than "request for changes" for just a trivial and purely code-smell review message, in order to be nice to new contributors.

[tanruixiang]

And in my opinion, I prefer just a comment rather than "request for changes" for just a trivial and purely code-smell review message, in order to be nice to new contributors.

Got it, thanks a lot for your suggestion. It should be that I misunderstood this "request for changes". I originally thought that only use "request for changes" can be clicked on github to modify. (Because if someone else suggests a little modification, I prefer to click on github to complete the modification🤣)

[tanruixiang]

Actually I like the original code more, since the iterator is used in the true-branch block to avoid a double search (the upper bound is actually not O(1) due to collision, and even O(1) DOES NOT imples no cost).

And I think this change does not improve readability, as the use of iterators is natural and easy to understand in c++. The false positives of cppcheck are entirely due to his own problems, and we should not modify the code logic just for that.

Maybe it's my bad habit, I always try not to use iterators in code snippets that are similar to algorithm problems. I will try to use iterators more in the future.

[PragmaTwice]

And in my opinion, I prefer just a comment rather than "request for changes" for just a trivial and purely code-smell review message, in order to be nice to new contributors.

Got it, thanks a lot for your suggestion. It should be that I misunderstood this "request for changes". I originally thought that only use "request for changes" can be clicked on github to modify. (Because if someone else suggests a little modification, I prefer to click on github to complete the modification🤣)

In the github review page, you can select "comment", "request for changes" or "approve". I think the first option is relatively "milder".

[manchurio]

@manchurio You can also consider using something like the following to encapsulate the logic with options and eliminate code duplication:

struct ZAddOptions {
  ZAddOptions() {}
  explicit ZAddOptions(uint8_t flags) : flags(flags) {}

  bool IsXX() const { return (flags & kZSetNX) != 0; }
  // other methods to ask if the specific flag was set

  uint8_t flags = 0;
};

Of course, you can add methods like SetXX to encapsulate set operations as well (to not spread bit operations over the codebase).

This refactoring has been completed

[torwig]

@manchurio Thank you, I'll have a look at it as soon as possible.

[tanruixiang]

Hi. Since there is a unified interface to get the flag's status, then we don't need additional variables to store. Even if there is reuse, there is no difference in speed.

[git-hulk]

Overall is good for me, only one comment.

[torwig]

LGTM

[PragmaTwice]

Sorry, touched wrong button. 🤣
The new branch generated by the revert button has been deleted.