Skip to content

Conversation

@raulgarciamsft
Copy link
Contributor

Closing the gap between Semmle & PreFAST
This rule is equivalent to C6248

raulgarciamsft and others added 2 commits September 20, 2018 16:28
Closing the gap between Semmle & PreFAST
This rule is equivalent to C6248
@jbj jbj added the C++ label Sep 21, 2018
@jbj
Copy link
Contributor

jbj commented Sep 21, 2018

There are some Unicode curly apostrophes (’ instead of ') in this PR that's causing our tests to fail due to platform differences. Please rebase to remove them from the git history.

I've spotted them in the following places

  • The commit message of the first commit.
  • The @name and @description of the query.
  • The two alert messages of the query.
  • In the UnsafeDaclSecurityDescriptor.expected file, where they've been encoded as \u2019.

@raulgarciamsft
Copy link
Contributor Author

Submitting fix. I will have to be careful in the future when using single quote ( ' ) to make sure my machine is not automatically replacing it.

@jbj
Copy link
Contributor

jbj commented Sep 21, 2018

I just noticed that some files use a mix of tabs and spaces. Sorry, but we have a CI check for that that only runs internally and not on external PRs. It'll start failing if we merge this. Please change tabs to spaces in all files.

Apart from that, this PR looks great, and I'll be happy to merge it. Thanks for the detailed help text and tests.

@jbj
Copy link
Contributor

jbj commented Sep 24, 2018

The tests are failing after 925c3b5 because all the line numbers in the expected file need to move down by one.

@raulgarciamsft
Copy link
Contributor Author

Fixing the .expected file and a few more tabs that I found.

@jbj
Copy link
Contributor

jbj commented Sep 25, 2018

The last fix to the expected file re-introduced to \u2019, which is why the test is failing now.

There's also now a conflict in .gitignore after I merged your other PR. I'm not sure where those .gitignore changes are coming from, but maybe we should just ignore all of /.vs?

@raulgarciamsft
Copy link
Contributor Author

Thanks… I have to find out why VS is changing the ' to ‘ … in the meantime I will try to get rid of them in the text.

Copy link
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution and the fixups! The tests are passing, except for a new test that's experimental for now and apparently broken.

@raulgarciamsft
Copy link
Contributor Author

Thanks a lot

aibaars pushed a commit that referenced this pull request Oct 14, 2021
Add `Ssa::WriteDefinition::assigns/1` predicate
smowton pushed a commit to smowton/codeql that referenced this pull request Feb 7, 2022
Kotlin: Fix build with old JDKs
MathiasVP pushed a commit to MathiasVP/ql that referenced this pull request Aug 10, 2025
Update go references in mod and sum files
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.

2 participants