sociomantic-tsunami / libdrizzle-redux

The next generation of Libdrizzle with a simplified API and support for more features of the protocol
Other
13 stars 12 forks source link

Add const to params for non-mutating functions #326

Open andreas-bok-sociomantic opened 5 years ago

andreas-bok-sociomantic commented 5 years ago

Adds const qualifier to POD params which are not mutated by the function.

Since it is (possibly) a breaking change for clients it is added to the milestone for the next major release

bokchan commented 5 years ago

const was added to all parameters which weren't mutated in the function. However as you note it might be redundant for the arguments passed by value.

bokchan commented 5 years ago

Seems there isn't a general agreement on whether to pass primitive types by const. It is recommended not to pass such as const ref, e.g. (const int &i).

Thus using const in cases here doesn't make much difference wrt. efficiency but does provide the clarity in the API that the function won't mutate the argument.

bokchan commented 5 years ago

Updated based on comments from @daniel-zullo-sociomantic

bokchan commented 5 years ago

Documentation needs to be updated

andreas-bok-sociomantic commented 5 years ago

Updated rst documentation

ben-palmer-sociomantic commented 5 years ago

The Use relnotes folder to describe new features commit seems unrelated to this PR. It could easily be merged on its own.

bokchan commented 5 years ago

The Use relnotes folder to describe new features commit seems unrelated to this PR. It could easily be merged on its own.

Hmm true. v7.x.x is behind v6.x.x but somehow I managed to get the commit with the relnotes folder onto it 🤦‍♂️ . I will block this until v6.2.0 has been released and v6.x.x has been merged into v7.x.x

codecov-io commented 5 years ago

Codecov Report

Merging #326 into v7.x.x will not change coverage. The diff coverage is 44.92%.