Skip to content

Conversation

@priyasiddharth
Copy link
Collaborator

other changes are format only

if (m_links.count(omni)) return *m_links.at(omni);

assert(false); // unreachable
assert(m_links.count(omni));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this change is important

Copy link
Contributor

Choose a reason for hiding this comment

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

the diff is too confusing. You are replacing a return with assert. That cannot be correct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The earlier code was

if (m_links.count(omni)) return *m_links.at(omni)
// else fall through to
assert(false)
// ** possibly undefined return value depending on semantics of assert **
// ** Thus, compiler complains.

Now it is rewritten such that intent is clearer and we pass the compiler check

assert(m_links.count(omni))
// ** possibly incorrect return value if assert is nop. However, **
// ** based on existing logic, we expect the assert to never fail **
return *m_links.at(omni)

ptr, {I.getOperand(1)});
auto *gep =
IRB.CreateGEP(ptr->getType()->getScalarType()->getPointerElementType(),
ptr, I.getOperand(1));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

important: braces around scalar initializer removed

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.

2 participants