forked from bitcoin/bitcoin
-
Notifications
You must be signed in to change notification settings - Fork 1
if on the same line
Vasil Dimov edited this page Jun 6, 2023
·
2 revisions
Reasons I prefer
// Case1
if (A) {
B;
}
over
// Case2
if (A) B;
-
Case1 is more debugger friendly - it is trivial to set a breakpoint on
B
. In Case2 if a breakpoint is set on that line, then the debugger will stop beforeA
is evaluated. Yes, it is possible to set a condition on the breakpoint to stop only ifA
is true, but that is more tedious and it meansA
will be evaluated two times which is problematic ifA
has side effects. -
Case1 is more diff friendly in case it has to be expanded:
// Case1
if (A) {
+ N;
B;
}
vs
// Case2
-if (A) B;
+if (A) {
+ N;
+ B;
+}
especially if A
and B
are complex expressions - the reviewer has to compare whether each one has been changed.
For example (this is real code):
- if (m_shift < n || m_shift >= std::numeric_limits<uint64_t>::max() || m_shift < std::numeric_limits<I>::min() || m_shift > std::numeric_limits<I>::max()) throw std::ios_base::failure("differential value overflow");
+ if (m_shift < n || m_shift >= std::numeric_limits<uint64_t>::max() || m_shift < std::numeric_limits<I>::min() || m_shift > std::numeric_limits<I>::max()) {
+ foo;
+ throw std::ios_base::failure("differential value overflow");
+ }
Was the if
condition changed?
What about this:
if (m_shift < n || m_shift >= std::numeric_limits<uint64_t>::max() || m_shift < std::numeric_limits<I>::min() || m_shift > std::numeric_limits<I>::max()) {
+ foo;
throw std::ios_base::failure("differential value overflow");
}