Skip to content

Conversation

aschackmull
Copy link
Contributor

This should be a clean refactor that doesn't change any behaviour or performance as it is simply moving some predicates around.

The upside of this refactor is that it reduces and simplifies the language-specific interface that's required for using call-context dependent virtual dispatch.

@hvitved
Copy link
Contributor

hvitved commented Jun 25, 2020

This should be a clean refactor that doesn't change any behaviour or performance as it is simply moving some predicates around.

Even though I agree with this, refactorings like these can sometimes make the QL evaluator reevaluate cached stages, so I think we should run Differences jobs just in case.

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

Changes LGTM, but the C++ code no longer compiles. Also, as I wrote I think we should run performance comparisons to be safe.

@aschackmull
Copy link
Contributor Author

@aschackmull
Copy link
Contributor Author

Differences jobs show no performance changes as expected.

@hvitved hvitved merged commit b57cfc9 into github:master Jun 30, 2020
@aschackmull aschackmull deleted the dataflow/dispatch-refactor branch June 30, 2020 06:29
max-schaefer pushed a commit to max-schaefer/codeql-go that referenced this pull request Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants