Skip to content

C++: Create tests readme. - #4719

Merged
criemen merged 7 commits into
github:mainfrom
geoffw0:issue84
Dec 4, 2020
Merged

C++: Create tests readme.#4719
criemen merged 7 commits into
github:mainfrom
geoffw0:issue84

Conversation

@geoffw0

@geoffw0 geoffw0 commented Nov 24, 2020

Copy link
Copy Markdown
Contributor

Add a README.md for the C/C++ CodeQL tests. See https://github.com/github/codeql-c-analysis-team/issues/84.

A specific goal is to document how we include declarations of standard library functions and types in the tests, without creating any dependencies (that might vary from machine to machine) or potential IP concerns. This policy was decided before Semmle was acquired by GitHub, and is a little strict IMO, but is still assumed to be current.

It's very much open to discussion what else should be in this document, and how it should be presented.

TODO:

  • get some early feedback
  • add examples
  • review from the doc team
  • review from legal? (even though this doc represents current practices, not new ones)

Comment thread cpp/ql/test/README.md Outdated
Comment thread cpp/ql/test/README.md Outdated
Comment thread cpp/ql/test/README.md Outdated
Comment thread cpp/ql/test/README.md Outdated

@jbj jbj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for writing this up!

Comment thread cpp/ql/test/README.md Outdated
Comment thread cpp/ql/test/README.md
@geoffw0

geoffw0 commented Nov 27, 2020

Copy link
Copy Markdown
Contributor Author

Who am I supposed to @ mention for a docs review?

@jbj

jbj commented Nov 30, 2020

Copy link
Copy Markdown
Contributor

Who am I supposed to @ mention for a docs review?

That's @hubwriter, according to CODEOWNERS. When you mark your PR as ready for review, CODEOWNERS (and the C team) will start getting notifications for this PR.

There is no code owner of *.md files, it seems. @hubwriter, do you want to own all Markdown files in this repo? Or just the ones in cpp/?

hubwriter
hubwriter previously approved these changes Nov 30, 2020

@hubwriter hubwriter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good.
I've made a few suggestions. Feel free to ignore these, of course.

Comment thread cpp/ql/test/README.md Outdated
Comment thread cpp/ql/test/README.md Outdated
Comment thread cpp/ql/test/README.md Outdated
Comment thread cpp/ql/test/README.md Outdated
Comment on lines +28 to +29
- ISO/IEC Programming languages - C (all versions)
- ISO/IEC Programming languages - C++ (all versions)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wasn't initially sure what these referred to. Maybe link "ISO/IEC Programming languages - C" to:
https://www.iso.org/standard/74528.html or https://www.iso.org/search.html?q=ISO/IEC%209899

and "ISO/IEC Programming languages - C++" to:
https://www.iso.org/standard/68564.html or https://www.iso.org/search.html?q=ISO/IEC%2014882

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I prefer the links to specific versions, even though they will become outdated, because as a user I'd prefer to be linked to something specific over a link to a search.

There are also draft versions, which I believe are free and thus probably quite popular in practice (despite potentially being inaccurate to the final standard).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the links.

Comment thread cpp/ql/test/README.md Outdated
Comment thread cpp/ql/test/README.md Outdated
Comment thread cpp/ql/test/README.md Outdated
Comment thread cpp/ql/test/README.md Outdated
Co-authored-by: hubwriter <hubwriter@github.com>
@geoffw0
geoffw0 marked this pull request as ready for review December 2, 2020 18:55
@geoffw0
geoffw0 requested a review from a team as a code owner December 2, 2020 18:55
@criemen
criemen merged commit 2ea9b4a into github:main Dec 4, 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.

5 participants