Skip to content

Conversation

aschackmull
Copy link
Contributor

Easy wins.

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Changes seem reasonable to me. Do you have any performance data / can you run DCA on this?

tausbn
tausbn previously approved these changes Oct 19, 2022
Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

Is assume_small_delta available in a currently-released version of the CodeQL CLI? If not, we might want to hold off on merging this until that is the case, so we don't suddenly break things for our users.

@aschackmull
Copy link
Contributor Author

can you run DCA on this

Will do.

Is assume_small_delta available in a currently-released version of the CodeQL CLI

It's currently bleeding edge, so you have to compile your own CLI to try it out, but it'll be in the next release. So yes, we should hold the merge until then.

@aschackmull
Copy link
Contributor Author

This should btw. be a noop in most common case (because the standard order won't be chosen), but it should measurably improve performance for queries with very large data flow graphs, i.e. on large databases or other otherwise poorly performing cases.

hvitved
hvitved previously approved these changes Oct 20, 2022
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.

Let's see what DCA says.

@aschackmull aschackmull dismissed stale reviews from hvitved and tausbn via 5b08814 October 20, 2022 12:27
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@aschackmull
Copy link
Contributor Author

Can't get dca for swift to work, but all other languages look uneventful, so I'd say LGTM from dca.

@aschackmull aschackmull merged commit 99ca28e into github:main Nov 7, 2022
@aschackmull aschackmull deleted the dataflow/joinorders branch November 7, 2022 10:05
owen-mc added a commit to owen-mc/codeql that referenced this pull request Nov 29, 2022
owen-mc added a commit to owen-mc/codeql that referenced this pull request Nov 29, 2022
owen-mc added a commit to owen-mc/codeql that referenced this pull request Nov 29, 2022
owen-mc added a commit to owen-mc/codeql that referenced this pull request Nov 29, 2022
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.

5 participants