Skip to content

Conversation

@asfernandes
Copy link
Member

No description provided.

src/jrd/fun.epp Outdated
Comment on lines 422 to 426
if (!(input && (input->dsc_flags & DSC_null)))
*arg_ptr++ = nullptr;
else
{
*arg_ptr++ = input;
}
*arg_ptr++ = input;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert the condition or maybe even simplify to:
*arg_ptr++ = (input && !input->isNull()) ? input : nullptr;


// select for ANY/ALL processing
const bool select_value = select_node->execute(tdbb, request);
const bool select_value = select_node->execute(tdbb, request).asBool();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The returned value is not used outside the IF condition, so I'd move the whole function call inside IF.

// look for a FALSE or null

if (!m_boolean->execute(tdbb, request))
if (m_boolean->execute(tdbb, request) != TriState(true))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (m_boolean->execute(tdbb, request) != TriState(true))
if (!m_boolean->execute(tdbb, request).asBool())

return false;

if (m_boolean && !m_boolean->execute(tdbb, request))
if (m_boolean && m_boolean->execute(tdbb, request) != TriState(true))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (m_boolean && m_boolean->execute(tdbb, request) != TriState(true))
if (m_boolean && !m_boolean->execute(tdbb, request).asBool())


// select for ANY/ALL processing
const bool select_value = select_node->execute(tdbb, request);
const bool select_value = select_node->execute(tdbb, request).asBool();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The returned value is not used outside the IF condition, so I'd move the whole function call inside IF.

if (m_inversion)
{
if (!m_condition || !m_condition->execute(tdbb, tdbb->getRequest()))
if (!m_condition || m_condition->execute(tdbb, tdbb->getRequest()) != TriState(true))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!m_condition || m_condition->execute(tdbb, tdbb->getRequest()) != TriState(true))
if (!m_condition || !m_condition->execute(tdbb, tdbb->getRequest()).asBool())

return false;

if (m_boolean && !m_boolean->execute(tdbb, request))
if (m_boolean && m_boolean->execute(tdbb, request) != TriState(true))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (m_boolean && m_boolean->execute(tdbb, request) != TriState(true))
if (m_boolean && !m_boolean->execute(tdbb, request).asBool())

return false;
}
if (!desc[1])
return !null2 && comparison < 0 ? TriState(false) : TriState::empty();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return !null2 && comparison < 0 ? TriState(false) : TriState::empty();
return (!null2 && comparison < 0) ? TriState(false) : TriState::empty();

return false;
}
return cmp1_3;
return null2 && cmp1_3 ? TriState::empty() : TriState(cmp1_3);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return null2 && cmp1_3 ? TriState::empty() : TriState(cmp1_3);
return (null2 && cmp1_3) ? TriState::empty() : TriState(cmp1_3);

if (blrOp == blr_containing)
return obj->contains(*tdbb->getDefaultPool(), str, strLen, patternStr, patternLen);
return TriState(obj->contains(*tdbb->getDefaultPool(),
str, strLen, patternStr, patternLen));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and below: please braces for the multi-line IF statement.

@asfernandes
Copy link
Member Author

@dyemanov do you think !value->execute(tdbb, request).asBool() is more clear than value->execute(tdbb, request) != TriState(true)?

For me, I understand the first only looking what isBool() does while the second is directly understandable as being NULL or FALSE (considering that is know that TriState does not implement SQL NULL logic).

@aafemt
Copy link
Contributor

aafemt commented Nov 6, 2025

More clear would be value->execute(tdbb, request).isNullOr(false). A little worse option from STL: !value->execute(tdbb, request).value_or(false).

@dyemanov
Copy link
Member

dyemanov commented Nov 6, 2025

@dyemanov do you think !value->execute(tdbb, request).asBool() is more clear than value->execute(tdbb, request) != TriState(true)?

For me, I understand the first only looking what isBool() does while the second is directly understandable as being NULL or FALSE (considering that is know that TriState does not implement SQL NULL logic).

Honestly, I don't have any strong preference. If I understand what asBool() means to return, I can also understand the reversed condition ;-) But generally I favor consistency and I've seen !asBool() used in the others parts of the patch, hence my suggestion. If you consider that != TriState(true) is clearer, I'm fine with that, but then please change the other occurrences of !asBool() to != TriState(true).

@asfernandes asfernandes self-assigned this Nov 7, 2025
@asfernandes asfernandes merged commit 53b218f into master Nov 7, 2025
45 of 46 checks passed
@asfernandes asfernandes deleted the work/req_null-2025 branch November 7, 2025 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants