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

Remove dead magics #13551

Merged
merged 2 commits into from
Mar 3, 2020
Merged

Remove dead magics #13551

merged 2 commits into from
Mar 3, 2020

Conversation

krux02
Copy link
Contributor

@krux02 krux02 commented Mar 1, 2020

I went through all the magics and verified if they are still used. If not all their code is deleted.

Note:

I deleted mSymDiffSet as it seems to be not used anymore. Is that a bug?

@krux02
Copy link
Contributor Author

krux02 commented Mar 1, 2020

test failure unrelated. It is yet again "https://github.com/status-im/nim-blscurve"

@krux02 krux02 requested a review from Araq March 1, 2020 21:48
@@ -370,7 +370,7 @@ type

const # magic checked op; magic unchecked op;
jsMagics: TMagicOps = [
["addInt", ""], # AddI
mAddI: ["addInt", ""],
Copy link
Member

Choose a reason for hiding this comment

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

how about using #11424 all the way, (ie replacing each comment by a litteral index as you started doing) => would guarantee it does the right thing at CT, especially as some magics are being removed

but this also could be done in later PR's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -1,18 +1,18 @@
type
int* {.magic: Int.} ## Default integer type; bitwidth depends on
int* {.magic: "Int".} ## Default integer type; bitwidth depends on
Copy link
Member

@timotheecour timotheecour Mar 2, 2020

Choose a reason for hiding this comment

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

we don't need 2 ways of doing the same thing but maybe the non string way is better (because simpler), ie some (future cleanup PR) could change these magics to use the non-string variant everywhere or even use a similar trick as importc and omit the name altogether when its derivable from the symbol name eg:

proc foo() {.magic: "Foo".} => `proc foo() {.magic.}`

Copy link
Member

Choose a reason for hiding this comment

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

The string version doesn't universally work, for string* {.magic: "string".} the compiler used to complain no string type was available for "string".

Copy link
Member

@timotheecour timotheecour Mar 2, 2020

Choose a reason for hiding this comment

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

The string version doesn't universally work

that doesn't seem incompatible with what i'm suggesting; Im suggesting:

  • change all string version to identifier versions
  • and optinally (like for importc), for the common case where identifier is predictable from symbol name (modulo capitalizeFirstLetter), skip it

type string* {.magic: String.} is already in non-string mode; and, under optional rule, would become type string* {.magic.} since "String"==capitalizeFirstLetter("string") ; note that the magic enum is still mString, that's not changing

proc `or`*(a, b: typedesc): typedesc {.magic: "TypeTrait", noSideEffect.}

would become (since optional shortcut doesn't apply):

proc `or`*(a, b: typedesc): typedesc {.magic: TypeTrait, noSideEffect.}

@krux02 krux02 force-pushed the remove-dead-magics branch from 15ae246 to d796b3e Compare March 2, 2020 10:36
@Araq Araq merged commit eb42f38 into nim-lang:devel Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants