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.

Sign up for GitHub

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

Jump to bottom

add pragma to params of macros.newProc #11025

Merged
merged 1 commit into from
Apr 27, 2019
Merged

add pragma to params of macros.newProc #11025

merged 1 commit into from
Apr 27, 2019

Conversation

loloicci
Copy link
Contributor

No description provided.

Araq
Araq reviewed Apr 14, 2019
View reviewed changes
lib/core/macros.nim Outdated
params: openArray[NimNode] = [newEmptyNode()];
body: NimNode = newStmtList();
procType = nnkProcDef;
pragma: NimNode = newEmptyNode()): NimNode {.compileTime.} =
Copy link
Member

Choose a reason for hiding this comment

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

It should be pragmas, plural.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean new commit or pragmas: openArray[NimNode] ?
If you mean later, should pragma be nnkIdent?

Copy link
Member

Choose a reason for hiding this comment

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

All I'm saying is that parameter's name shall be pragmas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.
Then, the newer commit is fixed one.
Thanks for retriggering CI.

@Araq
Copy link
Member

Araq commented Apr 14, 2019

Apart from my remark, very nice!

@loloicci loloicci force-pushed the add-pragma-params-of-new-proc branch from b1e5472 to a877aef Compare April 15, 2019 01:57
@loloicci
Copy link
Contributor Author

fixed.
thanks for your review!

@Araq Araq changed the title add progma to params of macros.newProc add pragma to params of macros.newProc Apr 15, 2019
@Araq Araq closed this Apr 15, 2019
@Araq Araq reopened this Apr 15, 2019
@loloicci
Copy link
Contributor Author

Failed test is related with my library nimly
There are some problems with it.
loloicci/nimly#11
loloicci/nimly#24
Are these related with this failure?

@Araq
Copy link
Member

Araq commented Apr 15, 2019

@narimiran are we sure nimly works on devel otherwise?

@narimiran
Copy link
Member

are we sure nimly works on devel otherwise?

I just checked and it seems to be wonky:

  • in 485d544 it worked
  • a commit after (0e6eb7d) if failed
  • a commit after (fdc3f54) if was ok again
  • then fail again, and ok after it

I'll investigate what is going on, and if I can't reproduce it maybe we should disable it until it is consistent.

(In the mean time, this can be merged)

@cooldome cooldome merged commit 6975554 into nim-lang:devel Apr 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Araq Araq Araq left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@loloicci @Araq @narimiran @cooldome