Skip to content

Swift: extract missing cases of AccessorKind and AccessSemantics enums #11450

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Nov 30, 2022

Conversation

d10c
Copy link
Contributor

@d10c d10c commented Nov 28, 2022

This resolves the "missing switch case" warnings that were showing up while compiling the swift extractor pack.

@d10c d10c requested a review from a team as a code owner November 28, 2022 12:18
@github-actions github-actions bot added the Swift label Nov 28, 2022
MathiasVP
MathiasVP previously approved these changes Nov 28, 2022
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM! I've been meaning to fix this for a long time. Thanks for doing it 🙇!

For future reference: When creating PRs that consist of both autogenerated and manually-written code it's super helpful when the PR is split into commits that are clearly marked as "this commit is totally autogenerated", or "this commit is totally handwritten".

@MathiasVP MathiasVP dismissed their stale review November 28, 2022 12:28

Oh, right. We need to write upgrade and downgrade scripts now. So we need to do that before we can merge it.

@d10c
Copy link
Contributor Author

d10c commented Nov 28, 2022

Right, I'll get onto the upgrade script. FYI the .cpp and .py files are the manually written ones.

@d10c d10c marked this pull request as draft November 28, 2022 18:07
@d10c d10c force-pushed the swift/missing-enum-cases branch from ba9c537 to 043e9a1 Compare November 28, 2022 18:33
@d10c
Copy link
Contributor Author

d10c commented Nov 28, 2022

Wrote the upgrade script. Though I'm not sure if it really works for updating old DBs, since I get the old "The database may be too new for the QL libraries the query is using; try upgrading them" error when following the testing steps here 🤔

@d10c d10c marked this pull request as ready for review November 28, 2022 18:35
d10c added 5 commits November 29, 2022 11:27
By initializing va_list the standard way,
i.e. leaving it uninitialized until va_start().
This resolves the warnings that were showing up during extractor-pack
compilation.
@d10c d10c force-pushed the swift/missing-enum-cases branch from 043e9a1 to 93cce0f Compare November 29, 2022 10:32
@d10c
Copy link
Contributor Author

d10c commented Nov 29, 2022

Updated tests + Accessor.toString(). Can confirm that both upgrade and downgrade scripts get applied according to the steps here.

@MathiasVP
Copy link
Contributor

LGTM once the test changes are accepted 😄.

Copy link
Contributor

@redsun82 redsun82 left a comment

Choose a reason for hiding this comment

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

LGTM!

@AlexDenisov
Copy link
Contributor

I also started an experiment here just to be sure there are no surprises https://github.com/github/codeql-dca-main/issues/8905

@MathiasVP
Copy link
Contributor

DCA looks fine. Merging!

@MathiasVP MathiasVP merged commit d53d275 into github:main Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants