C++: A few IR QLDoc comments #3393

dbartol · 2020-05-01T22:19:02Z

A small part of https://github.com/github/codeql-c-analysis-team/issues/40

rdmarsh2 · 2020-05-01T23:15:53Z

cpp/ql/src/semmle/code/cpp/ir/implementation/TempVariableTag.qll

 private import internal.TempVariableTagInternal
 private import Imports::TempVariableTag

+/**
+ * Describes the reason that a particular IR temporary variable was generated. For example, it could


QLDoc for a class is usually a singular noun phrase. I'm not clear on whether that's a style guide requirement or not. All the examples use the "An expression..." style, but it's not explicitly written.

Suggested change

* Describes the reason that a particular IR temporary variable was generated. For example, it could

* A reason that a particular IR temporary variable was generated. For example, it could

Yes, we use A/An. See https://wiki.semmle.com/pages/viewpage.action?pageId=74613282#QLDocStyleGuide-QLDocforclasses (linked from https://github.com/github/codeql-c-analysis-team/issues/40). That part of the style guide hasn't made it to the public repo yet,.

felicitymay

I've made a whole bunch of suggestions that aim to make the text clearer. It's possible that I've not correctly understood the intention of some comments.

I stopped when I got to what appear to be duplicate entries.

felicitymay · 2020-05-04T10:12:07Z

cpp/ql/src/semmle/code/cpp/ir/implementation/EdgeKind.qll

+ * Gets the single instance of `GotoEdge`, representing the unconditional successor of an
+ * `Instruction`


Suggested change

* Gets the single instance of `GotoEdge`, representing the unconditional successor of an

* `Instruction`

* Gets the single instance of `GotoEdge` that represents the unconditional successor of an

* `Instruction`.

Would your suggestion still stand if I clarified that there is only a single GotoEdge object in the entire library, and that that single instance is used in calls like:

exists(Instruction instruction, Instruction successor | instruction.getSuccessor(gotoEdge()) = successor )

?

In that case, perhaps:

Suggested change

* Gets the single instance of `GotoEdge`, representing the unconditional successor of an

* `Instruction`

* Gets the single instance of `GotoEdge`. This instance represents the unconditional successor of an

* `Instruction`.

But it's definitely at times like this where my lack of programming knowledge is a hinderance.

After a bit of discussion in Slack, I've moved these predicates into an EdgeKind module (to match the EdgeKind class with which they are associated), and shortened the QLDoc comments to just say "Gets the single instance of the FooEdge class.". The documentation for the FooEdge class itself explains what that is.

felicitymay · 2020-05-04T10:12:42Z

cpp/ql/src/semmle/code/cpp/ir/implementation/EdgeKind.qll

@@ -38,6 +43,10 @@ class TrueEdge extends EdgeKind, TTrueEdge {
  final override string toString() { result = "True" }
 }

+/**
+ * Gets the single instance of `TrueEdge`, representing the successor of a conditional branch when


Suggested change

* Gets the single instance of `TrueEdge`, representing the successor of a conditional branch when

* Gets the single instance of `TrueEdge` that represents the successor of a conditional branch when

felicitymay · 2020-05-04T10:13:08Z

cpp/ql/src/semmle/code/cpp/ir/implementation/EdgeKind.qll

@@ -48,6 +57,10 @@ class FalseEdge extends EdgeKind, TFalseEdge {
  final override string toString() { result = "False" }
 }

+/**
+ * Gets the single instance of `FalseEdge`, representing the successor of a conditional branch when


Suggested change

* Gets the single instance of `FalseEdge`, representing the successor of a conditional branch when

* Gets the single instance of `FalseEdge` that represents the successor of a conditional branch when

felicitymay · 2020-05-04T10:17:41Z

cpp/ql/src/semmle/code/cpp/ir/implementation/internal/OperandTag.qll

+   * Gets an integer representing the order in which this operand should appear in the operand list
+   * of an instruction when printing the IR.


Not entirely sure whether I've understood this correctly. Perhaps?

Suggested change

* Gets an integer representing the order in which this operand should appear in the operand list

* of an instruction when printing the IR.

* Gets an integer that represents where this operand will appear in the operand list

* of an instruction when the IR is printed.

felicitymay · 2020-05-04T10:18:24Z

cpp/ql/src/semmle/code/cpp/ir/implementation/internal/OperandTag.qll

  abstract int getSortOrder();

+  /**
+   * Gets a label that will appear before the operand when printing the IR.


Suggested change

* Gets a label that will appear before the operand when printing the IR.

* Gets a label that will appear before the operand when the IR is printed.

felicitymay · 2020-05-04T10:31:39Z

cpp/ql/src/semmle/code/cpp/ir/implementation/EdgeKind.qll

+ * Gets the `CaseEdge` representing the successor of a `Switch` instruction corresponding to the
+ * `case` label with the specified lower and upper bounds.


Not sure what this means.

I've expanded the text and included an example.

felicitymay · 2020-05-04T10:33:01Z

csharp/ql/src/semmle/code/csharp/ir/implementation/EdgeKind.qll

@@ -28,6 +29,10 @@ class GotoEdge extends EdgeKind, TGotoEdge {
  final override string toString() { result = "Goto" }
 }

+/**
+ * Gets the single instance of `GotoEdge`, representing the unconditional successor of an


Suggested change

* Gets the single instance of `GotoEdge`, representing the unconditional successor of an

* Gets the single instance of `GotoEdge` that represents the unconditional successor of an

felicitymay · 2020-05-04T10:33:28Z

csharp/ql/src/semmle/code/csharp/ir/implementation/EdgeKind.qll

@@ -38,6 +43,10 @@ class TrueEdge extends EdgeKind, TTrueEdge {
  final override string toString() { result = "True" }
 }

+/**
+ * Gets the single instance of `TrueEdge`, representing the successor of a conditional branch when


Suggested change

* Gets the single instance of `TrueEdge`, representing the successor of a conditional branch when

* Gets the single instance of `TrueEdge` that represents the successor of a conditional branch when

felicitymay · 2020-05-04T10:34:07Z

csharp/ql/src/semmle/code/csharp/ir/implementation/EdgeKind.qll

@@ -48,6 +57,10 @@ class FalseEdge extends EdgeKind, TFalseEdge {
  final override string toString() { result = "False" }
 }

+/**
+ * Gets the single instance of `FalseEdge`, representing the successor of a conditional branch when


Suggested change

* Gets the single instance of `FalseEdge`, representing the successor of a conditional branch when

* Gets the single instance of `FalseEdge` that represents the successor of a conditional branch when

felicitymay · 2020-05-04T10:58:13Z

csharp/ql/src/semmle/code/csharp/ir/implementation/EdgeKind.qll

@@ -58,6 +71,10 @@ class ExceptionEdge extends EdgeKind, TExceptionEdge {
  final override string toString() { result = "Exception" }
 }

+/**
+ * Gets the single instance of `ExceptionEdge`, representing the successor of an instruction when
+ * that instruction's evaluation throws an exception.


See cpp comment above.

Yeah, sorry for all the duplicate code. It's an artifact of how we share the IR code with C#. We have a script and PR check that keeps them all in sync, so once I make the change in one place, it will be propagated to all the other duplicate copies as well.

Sorry - it wasn't intended to be complaint - more an explanation of why I'd stopped commenting 😄

Note that the various predicates to access the singleton instances of the `EdgeKind` classes have been moved into a module named `EdgeKind`.

dbartol · 2020-05-06T21:30:38Z

OK, I think this is ready for final review now.

felicitymay

Thanks for the changes. 👍
It's nice to see a new example in the library comments 😄

dbartol · 2020-05-11T12:38:09Z

@github/codeql-c-analysis Can I get an official approval?

C++: A few IR QLDoc comments

a166a4d

dbartol added C++ documentation labels May 1, 2020

dbartol requested review from jbj and rdmarsh2 May 1, 2020 22:19

rdmarsh2 reviewed May 1, 2020

View reviewed changes

rdmarsh2 requested a review from a team May 1, 2020 23:16

felicitymay reviewed May 4, 2020

View reviewed changes

C++: Fix PR feedback

df4fdaf

Note that the various predicates to access the singleton instances of the `EdgeKind` classes have been moved into a module named `EdgeKind`.

dbartol requested review from a team as code owners May 6, 2020 21:06

C++: Add missing module comment

f0e86a9

felicitymay previously approved these changes May 7, 2020

View reviewed changes

C++/C#: Fix formatting

e435484

dbartol dismissed felicitymay’s stale review via e435484 May 7, 2020 12:39

jbj approved these changes May 11, 2020

View reviewed changes

jbj merged commit cec73e6 into github:master May 11, 2020

	* Describes the reason that a particular IR temporary variable was generated. For example, it could
	* A reason that a particular IR temporary variable was generated. For example, it could

		* Gets the single instance of `GotoEdge`, representing the unconditional successor of an
		* `Instruction`

	* Gets the single instance of `TrueEdge`, representing the successor of a conditional branch when
	* Gets the single instance of `TrueEdge` that represents the successor of a conditional branch when

	* Gets the single instance of `FalseEdge`, representing the successor of a conditional branch when
	* Gets the single instance of `FalseEdge` that represents the successor of a conditional branch when

		* Gets an integer representing the order in which this operand should appear in the operand list
		* of an instruction when printing the IR.

	* Gets a label that will appear before the operand when printing the IR.
	* Gets a label that will appear before the operand when the IR is printed.

		* Gets the `CaseEdge` representing the successor of a `Switch` instruction corresponding to the
		* `case` label with the specified lower and upper bounds.

C++: A few IR QLDoc comments #3393

C++: A few IR QLDoc comments #3393

Uh oh!

Conversation

dbartol commented May 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felicitymay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbartol May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbartol commented May 6, 2020

Uh oh!

felicitymay left a comment

Choose a reason for hiding this comment

Uh oh!

dbartol commented May 11, 2020

Uh oh!

Uh oh!

dbartol May 4, 2020 •

edited

Loading