[bind10-dev] style thingy wrt unit test code coverage

[Shane Kerr]

Evan,

On Tue, 2010-02-23 at 18:45 +0000, Evan Hunt wrote:
> > if (condition) { do_something; }
> > 
> > on one line will always be counted, because the condition is executed,
> > but we might miss the fact that do_something might not be.
> > 
> > I don't know about the rest but i tend to put simple things like this on
> > the same line, and perhaps we should make it a style issue to not do
> > that, as the coverage result gets skewed.
> 
> I think with if statements, that's already covered in the BIND9 style
> guide--and yeah, we shouldn't do it.

The only time I ever do that is with early-exit goto statements, which I
would probably use an exception for in almost every case in C++, so for
me personally I am happy with always making conditionals 2 or more
lines.

> > The more interesting ones is the same issue in header files; 'simple'
> > functions we can do there, but as soon as branching gets involved can we
> > still call it simple?
> 
> IMHO it's okay to have a one-line declaration that does a simple return
> or assignment:
> 
>         const int foo() const { return foo_; }
>         void setFoo(int f) { foo_ = f; }
> 
> ...but anything more complicated than that should be multi-line.

This also makes sense to me.

> Speaking of bracket style, there doesn't seem to be any agreement on when
> to open a bracket at the end of a line and when to move it down to a new
> line.  I find this rather ugly:
> 
>     DNSKEYImpl(uint16_t flags, uint8_t protocol, uint8_t algorithm,
>                const vector<char>& keydata) :
>         flags_(flags), protocol_(protocol), algorithm_(algorithm),
>         keydata_(keydata)
>     {
>         ...
>     }
> 
> ...but better on the whole than leaving the bracket on the line above,
> causing the "keydata_(keydata)" line to be indented at the same level
> as code immediately below it:
> 
>     DNSKEYImpl(uint16_t flags, uint8_t protocol, uint8_t algorithm,
>                const vector<char>& keydata) :
>         flags_(flags), protocol_(protocol), algorithm_(algorithm),
>         keydata_(keydata) {
>         ...
>     }

Agreed. I thought the first case (open squiggly-brace on a line by
itself) was the rule? Anyway, I am happy with that, but not so worried
about either.

> And, speaking of indentation style and how it interacts with bracket style,
> long if statements are problematic with our standard four-space indent:
> 
>     if (impl_->flags_ != other_dnskey.impl_->flags_ &&
>         impl_->protocol_ != other_dnskey.impl_->protocol_ &&
>         impl_->algorithm_ != other_dnskey.impl_->algorithm_) {
>         do_some_stuff();
>     }
> 
> Two proposed solutions:
> 
>     if (impl_->flags_ != other_dnskey.impl_->flags_ &&
>            impl_->protocol_ != other_dnskey.impl_->protocol_ &&
>            impl_->algorithm_ != other_dnskey.impl_->algorithm_) {
>         do_some_stuff();
>     }
> 
>     if (impl_->flags_ != other_dnskey.impl_->flags_ &&
>         impl_->protocol_ != other_dnskey.impl_->protocol_ &&
>         impl_->algorithm_ != other_dnskey.impl_->algorithm_)
>     {
>         do_some_stuff();
>     }
> 
> Shall we agree on one of these?  (As a matter of taste, I think the first
> one is slightly more attractive, but they both work okay.)

I don't care too much, but I prefer the second. I like lots of vertical
space in my code though.

--
Shane