Skip to content

CPP: Add syntax examples to QLDoc in Class.qll.#1570

Merged
jbj merged 9 commits intogithub:masterfrom
geoffw0:qldoceg
Aug 6, 2019
Merged

CPP: Add syntax examples to QLDoc in Class.qll.#1570
jbj merged 9 commits intogithub:masterfrom
geoffw0:qldoceg

Conversation

@geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Jul 9, 2019

This is my first PR for https://jira.semmle.com/browse/CPP-407 ('Add concrete syntax examples to QLDoc of all AST leaf classes'). I highly encourage discussion and suggestions on what I've done here - whatever we settle on will become a template for how we do this everywhere else (in CPP).

I'm also coming to realize that 'leaf classes' doesn't mean as much as I thought it did, as very often we have a QL class A with child classes B and C extending it, but the union of B and C doesn't cover all (very often not even most) instances of A. Thus I feel a need to document A in addition to B and C, and the scope of this task increases. Please discuss which classes you think should and shouldn't get examples (though I probably won't delete any that I've already written).

@geoffw0 geoffw0 added the C++ label Jul 9, 2019
@geoffw0 geoffw0 requested a review from a team as a code owner July 9, 2019 14:16
Copy link
Contributor

@ian-semmle ian-semmle left a comment

Choose a reason for hiding this comment

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

Great stuff! I've added a few comments with thoughts on the details.

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.

Overall this looks good, and I think it's what we want. I agree with most of Ian's comments, and I have some additional comments as well. I'll hold this PR to a higher standard than the subsequent ones since you suggested that this one establishes the best practices we'll be following later.

The QLDoc for ClassDerivation could use some backticks and consistent brace placement.

This file, like others, contains a number of QL classes that are specific to a query or style guide and probably shouldn't have been put here in the first place. For example, class Interface is not a standard C++ thing and doesn't even seem to be in use. I'm fine with not putting examples on such classes, and you could even take this opportunity to deprecate some of them.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jul 11, 2019

The QLDoc for ClassDerivation could use some backticks and consistent brace placement.

Done.

class Interface is not a standard C++ thing and doesn't even seem to be in use. I'm fine with not putting examples on such classes, and you could even take this opportunity to deprecate some of them.

I've deprecated Interface.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Aug 6, 2019

I believe this is ready to merge.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Aug 6, 2019

Rebased, merge fixed.

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.

LGTM

@jbj jbj merged commit 077f372 into github:master Aug 6, 2019
@geoffw0 geoffw0 deleted the qldoceg branch May 10, 2022 08:47
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.

3 participants