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

typeof(voidStmt) now works #17807

Merged
merged 7 commits into from
Apr 23, 2021
Merged

typeof(voidStmt) now works #17807

merged 7 commits into from
Apr 23, 2021

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Apr 21, 2021

preferably merge #17813 before this so that CI is green (EDIT: ended up pushing a dummy commit to rerun CI so that it's green because #17813 wasn't merged yet)

this now works:

proc fn() = discard
assert typeof(fn()) is void

likewise with other statements that return void, see tests.

I also added a -d:nimHasTypeofVoid condsyms

note

only 1 place was broken (in cligen) and that was because it used when compiles(type(p)), which is the very thing i'm fixing here. It affected 2 packages (nimterop, prologue) but both orginated from this line; i've sent out c-blake/cligen#193 to fix this with a fwd and backward compatible change, and it was merged (and tagged, which was needed), fixing those 2 packages.

added new PTAL label to help triage issues where ball is in reviewer's camp

@Araq I've added a label PTAL, this is something i've wanted to add for a while and discussed with @xflywind before, the intent is to indicate that a PR is ready for (first or next) round of reviews, meaning some reviewer action is needed, eg:

  • all comments are addressed or some comments need reviewer action
  • CI is either green or only contains CI failures unrelated to PR (or a [skip ci] was passed before followup #17561, skipping ci now implies green #17813) makes CI for azure red; the benefit being: faster turnaround and avoid wasting CI resources when a commit is deemed safe, eg changelog entry or comment that was verified locally to work with nim doc)
  • the PR author, reviewer or anyone can remove the PTAL label when the ball is in PR author's camp

(this feature exists in some form or another in other code review tools)

The goal of PTAL label is to make it easier to identify which PR's need reviewer attention (via github labels) without requiring annoying pings; simply looking at green/red CI status is not a good indication for that (because CI can be green but with unaddressed review comments, or red but still mergeable because of [skip ci] and unrelated CI failures). Of course, both PR author and reviewer need to make sure that this is indeed the case.

example

as you can see in this PR, last commit made it red (changelog only entry with [skip ci] before #17813) but previous commit was green, therefore it is mergeable:

image

future work

when true:
  var a = 1
  type B = typeof((let a = 2; a))

(see also D20210421T014713 in this PR)

  • consider whether we also want this to work:
proc fn() = discard
assert fn() is void

@timotheecour timotheecour force-pushed the pr_typeof_void branch 2 times, most recently from 0c72262 to facb97a Compare April 21, 2021 08:28
@timotheecour timotheecour changed the title make typeof(stmt) (returning void) work typeof(voidStmt) now works Apr 21, 2021
@timotheecour timotheecour marked this pull request as ready for review April 21, 2021 17:20
@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label Apr 21, 2021
@Araq Araq merged commit 2abc936 into nim-lang:devel Apr 23, 2021
@timotheecour timotheecour deleted the pr_typeof_void branch April 23, 2021 18:16
@timotheecour timotheecour added TODO: followup needed remove tag once fixed or tracked elsewhere and removed Ready For Review (please take another look): ready for next review round labels Apr 23, 2021
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
* `typeof(voidStmt)` now works
* remove typeOrVoid
* add condsyms, and reference cligen c-blake/cligen#193
* fixup
* changelog [skip ci]
* fixup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: followup needed remove tag once fixed or tracked elsewhere
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants