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

getCustomPragma is split up in more usable chunks #11526

Merged
merged 14 commits into from
Apr 14, 2021

Conversation

krux02
Copy link
Contributor

@krux02 krux02 commented Jun 17, 2019

rewrote the customPragma macros for more mudular use.

@krux02 krux02 closed this Jun 19, 2019
@krux02 krux02 reopened this Jun 19, 2019
@narimiran narimiran closed this Jun 20, 2019
@narimiran narimiran reopened this Jun 20, 2019
@krux02 krux02 force-pushed the split-custom-pragam-macro branch from 4dd8121 to 4debbb9 Compare June 20, 2019 20:33
@krux02 krux02 closed this Jul 2, 2019
@krux02 krux02 reopened this Jul 2, 2019
@Araq
Copy link
Member

Araq commented Jul 3, 2019

This needs a [feature] tag and a changelog entry.

@dom96
Copy link
Contributor

dom96 commented Aug 11, 2019

Here is the stack trace I get when using this code in my serializer: https://gist.github.com/dom96/af8e3e919d2bb7ed503a4356913faea9

@krux02
Copy link
Contributor Author

krux02 commented Aug 11, 2019

I don't get it. When I run all tests locally, there is one test failing in IC. But when I run category IC only, then there is nothing failing.

output of all tests: http://ix.io/1RkI (fail in category IC)
output of IC only: http://ix.io/1RkQ (success)

Also the CI integration here on the servers tells a very different story.

@planetis-m
Copy link
Contributor

Awesome, finally someone made it a compile time proc instead of a macro. @krux02 <3

@krux02
Copy link
Contributor Author

krux02 commented Nov 6, 2019

@B3liever yes that was the intention.

@krux02
Copy link
Contributor Author

krux02 commented Apr 6, 2020

@Araq, this PR should have been merged long ago.

@Araq
Copy link
Member

Araq commented Apr 7, 2020

No RFC process, no explanation what it does. It should have had an explanation a long time ago.

@krux02
Copy link
Contributor Author

krux02 commented Apr 7, 2020

@Araq: This PR enables to read custom pragma annotations from the Nim macro system.

lib/core/macros.nim Outdated Show resolved Hide resolved
lib/core/macros.nim Outdated Show resolved Hide resolved
lib/core/macros.nim Outdated Show resolved Hide resolved
@Clyybber
Copy link
Contributor

I'm taking over

@timotheecour
Copy link
Member

@Clyybber IIUC this duplicates the logic in the compiler, which is guaranteed to be fragile; instead why not simply add a magic to avoid duplicating logic and guaranteeing results match what the compiler does?

@zah
Copy link
Member

zah commented May 25, 2020

@Clyybber, if you are interested in getting this to work, you might try applying it here:
https://github.com/status-im/nim-serialization/blob/master/serialization/object_serialization.nim#L43-L49

I've found getCustomPragma to be quite broken in real-world usages in generic code, so I've created a different in-house implementation that has a much simpler implementation, but a more cumbersome API.

You can consider getCustomPragma working if the test suite of nim-json-serialization executes sucessfully after the modification.

@Clyybber
Copy link
Contributor

Clyybber commented May 25, 2020

@timotheecour semCustomPragma shares almost no logic with this. The complex parts here are getting to where the pragma actually is.

@zah After 5cde03e it works fine (testsuite of nim-json-serialization passes); I replaced the code snippet you mentioned with

  for fieldName, fieldVar in fieldPairs(obj):
    dumpTree fieldVar
    when not hasCustomPragma(fieldVar, dontSerialize):
      when hasCustomPragma(fieldVar, serializedFieldName):
        const fieldNameVar = getCustomPragmaVal(fieldVar, serializedFieldName)
      else:
        const fieldNameVar = fieldName
      body

@zah
Copy link
Member

zah commented May 26, 2020

@Clyybber, nice! Looking forward to start using this in the next release

@disruptek
Copy link
Contributor

Is this going in soon?

@krux02
Copy link
Contributor Author

krux02 commented Jun 18, 2020

Celebrating first anniversary of unmerged but finished PR. Test failure unrelated.

of nnkSym:
result = getCustomPragmaNode(typeSym, name)
of nnkProcTy:
# It is a bad idea to support this. The annotation can't be part
Copy link
Contributor

Choose a reason for hiding this comment

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

Then can we not support it?

lib/core/macros.nim Outdated Show resolved Hide resolved
Co-authored-by: Andreas Rumpf <rumpf_a@web.de>
@Araq Araq merged commit 56c3775 into nim-lang:devel Apr 14, 2021
@Clyybber Clyybber mentioned this pull request Jul 29, 2021
@Clonkk Clonkk mentioned this pull request Jan 20, 2022
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
* getCustomPragma is split up in more usable chunks
* changelog entry
* fix for style checks
* shitty typedesc special casing
* Add since annotation and remove typedesc comments
* Fix typo
* Revert since annotation because it breaks bootstrapping
* Export getCustomPragmaNode conditionally
* Reduce code duplication
* Update since
* Update lib/core/macros.nim
* Apply suggestions from code review

Co-authored-by: Clyybber <darkmine956@gmail.com>
Co-authored-by: Andreas Rumpf <rumpf_a@web.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants