Skip to content
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

Add lgtm explanation #4362

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Add lgtm explanation #4362

wants to merge 1 commit into from

Conversation

nlohmann
Copy link
Owner

Closes #4361 by adding a comment why goto should not be considered harmful in the number parser.

@nlohmann nlohmann added this to the Release 3.11.4 milestone Apr 29, 2024
@nlohmann nlohmann self-assigned this Apr 29, 2024
@github-actions github-actions bot added the S label Apr 29, 2024
@coveralls
Copy link

Coverage Status

coverage: 100.0%. remained the same
when pulling 2785437 on lgtm-goto-explanation
into 8c391e0 on develop.

@TheJCAB
Copy link
Contributor

TheJCAB commented May 1, 2024

Thanx!

Note that I don't know if a justification in the old style suppression format will be accepted. The internal guide to resolve the issue just mentions the "// CodeQL [query-id] Justification" format, which IIUC needs to be in a comment on its own line (not appended to the code). From my searches in the codeql public documentation, the only mention of suppressions (old or new) is in the release notes of CodeQL 2.12.0 announcing support for the new format, and from looking at documentation for the CodeQL scanning offered here on GitHub, they seem to be using 2.17.0, so that should work fine. I'd encourage you to try it.

@TheJCAB
Copy link
Contributor

TheJCAB commented May 15, 2024

I opened support requests on both CodeQL here on github and on the internal team managing CodeQL policies, but I've heard nothing from either.

I say go ahead and complete this as-is. It's an improvement and I expect it will suffice. If changing the suppression style becomes needed, it'll be a problem for another day.

Thank you!

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.

CodeQL suppressions lack justification
3 participants