Sitelet https://web.archive.org/web/20260124011818/https://github.com/github/codeql/pull/4178
Skip to content

Conversation

@igfoo
Copy link
Member

@igfoo igfoo commented Sep 1, 2020

No description provided.

@igfoo igfoo added the C++ label Sep 1, 2020
@igfoo igfoo requested a review from a team as a code owner September 1, 2020 11:55
@igfoo igfoo added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Sep 1, 2020
@igfoo igfoo force-pushed the igfoo/48-coroutine-support-3 branch from 62cbd25 to 65c8957 Compare September 1, 2020 12:23
jbj
jbj previously approved these changes Sep 1, 2020
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. We can add CFG and IR support in follow-up PRs.


override string getOperator() { result = "co_yield" }

override int getPrecedence() { result = 2 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do you find these precedence numbers?

Copy link
Member Author

Choose a reason for hiding this comment

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

The spec doesn't explicitly give the precedence; it comes out of the grammar. You can also compare with https://en.cppreference.com/w/cpp/language/operator_precedence where we put e.g. AddressOfExpr and CoAwaitExpr in the same bucket, and Assignment and CoYieldExpr in the same bucket, although we number the buckets differently.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@intrigus-lgtm intrigus-lgtm left a comment

Choose a reason for hiding this comment

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

"Nice to have" But not terrible useful either.


/**
* A C/C++ co_await expression
* ```
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* ```
* ```cpp

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, but I think that for consistency, if we're going to do this then we should do it for all QL examples at once, in a separate PR, rather than just doing it for these new ones here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can understand your thought, but at the same time doing that for all QL examples would be a huge amount of work.
I know that only doing this for new code leads to inconsistency, but if you never start you just have more work for when you also do the other examples.
Anyway, feel free to ignore my suggestions :)


/**
* A C/C++ co_yield expression
* ```
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* ```
* ```cpp

* A C/C++ 'co_return' statement.
*
* For example:
* ```
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* ```
* ```cpp

* co_return 1+2;
* ```
* or
* ```
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* ```
* ```cpp

* Gets the operand of this 'co_return' statement.
*
* For example, for
* ```
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* ```
* ```cpp

* co_return 1+2;
* ```
* the operand is a function call `return_value(1+2)`, and for
* ```
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* ```
* ```cpp

* co_return 1+2;
* ```
* the result is `1+2`, and there is no result for
* ```
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* ```
* ```cpp

* Gets the expression of this 'co_return' statement, if any.
*
* For example, for
* ```
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* ```
* ```cpp

* Holds if this 'co_return' statement has an expression.
*
* For example, this holds for
* ```
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* ```
* ```cpp

* co_return 1+2;
* ```
* but not for
* ```
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* ```
* ```cpp

@igfoo igfoo force-pushed the igfoo/48-coroutine-support-3 branch 2 times, most recently from 6a4cb80 to c3fb584 Compare September 3, 2020 19:16
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

My comments are nitpicky, this LGTM.

It would be good at some point soon to have some complete test cases where coroutines are used, i.e. both implemented and called in a natural way. I think I know what this would look like but I haven't ever used coroutines myself.

}

/**
* A C/C++ co_await expression
Copy link
Contributor

Choose a reason for hiding this comment

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

backticks around co_await (and other similar cases).

}

/**
* A C/C++ 'co_return' statement.
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be backticks.

@igfoo
Copy link
Member Author

igfoo commented Sep 4, 2020

There are a few tests for this in the extractor testsuite.

@igfoo igfoo force-pushed the igfoo/48-coroutine-support-3 branch from d2b69f6 to d49bc4c Compare September 7, 2020 19:54
@nickrolfe nickrolfe merged commit 075ce6e into main Sep 8, 2020
@nickrolfe nickrolfe deleted the igfoo/48-coroutine-support-3 branch September 8, 2020 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants