Skip to content

Revert "Revert "C++: Work around extractor issue CPP-383""#5696

Merged
jbj merged 3 commits into
github:mainfrom
jbj:reapply-inconsistency-workaround
Apr 23, 2021
Merged

Revert "Revert "C++: Work around extractor issue CPP-383""#5696
jbj merged 3 commits into
github:mainfrom
jbj:reapply-inconsistency-workaround

Conversation

@jbj

@jbj jbj commented Apr 16, 2021

Copy link
Copy Markdown
Contributor

Revert the revert of the workaround for CFG issues when a FunctionCall has a getTarget that does not exist. While we've fixed the main cause of the problem, it can apparently still happen in rare cases as a result of extractor crashes.

This reverts commit ee5eaef.

Cc @lcartey.

**Revert the revert** of the workaround for CFG issues when a
`FunctionCall` has a `getTarget` that does not exist. While we've fixed
the main cause of the problem, it can apparently still happen in rare
cases as a result of extractor crashes.

This reverts commit ee5eaef.
@jbj jbj added the C++ label Apr 16, 2021
@jbj jbj requested a review from a team as a code owner April 16, 2021 14:49

/**
* Holds if `fc` is a `FunctionCall` with no return value for `getTarget`. This
* can happen due to extractor issue CPP-383.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we link to the new issue from Luke here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The problem of no getTarget value can in principle be caused by any extractor crash, so I've changed this comment to avoid mentioning a specific issue.

)
}

// This base case is pulled out to work around QL-796

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we have a GH issue for this instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We do now. I've updated all references to QL-796. We decided on Slack that it's okay to link to this internal issue from code.

@criemen criemen added the no-change-note-required This PR does not need a change note label Apr 16, 2021
@jbj jbj requested a review from a team as a code owner April 23, 2021 07:59
@github-actions github-actions Bot added the C# label Apr 23, 2021
@jbj jbj assigned criemen and unassigned igfoo Apr 23, 2021

@criemen criemen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@jbj jbj merged commit 9b5bb95 into github:main Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C# C++ no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants