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

nimony: subscripts sans generic instantiations #194

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Dec 12, 2024

Code is messy for now, will move out the bugfix addParLes and do the ArrayT reorder as mentioned in #192 first. Also a bug that let arrIndex1: int = x[0] gives got: (i -1) but wanted: (u -1)

@Araq
Copy link
Member

Araq commented Dec 12, 2024

I would have done it this way:

proc buildReadAccessor(c: var CheckContext; dest: var Tree; tree: Tree, n: NodePos, acc: string) =
  copyIntoKind(dest, Call, n.info):
    let accessor = c.m.strings.getOrIncl(acc)
    buildSymChoice(c, dest, accessor, n.info)
    for ch0 in sonsReadonly(tree, n):
      copyTree(dest, tree, ch0)

proc semBracketExpr(c: var SemContext; dest: var Tree; tree: Tree; n: NodePos;
                    preferredType: FullTypeId): FullTypeId =
  var tmp = createTempTree(c.thisModule)
  discard sem(c, tmp, tree, n.firstSon, autoType)
  var routines: seq[FullSymId] = @[]
  if isTypeExpr(c, tmp, StartPos):
    rollbackTo c.p, tmp, StartPos
    result = borrowPosition(c, dest)
    semDeclType(c, dest, tree, n, inLocalDecl)
  elif isGenericRoutine(c, tmp, StartPos, routines):
    rollbackTo c.p, tmp, StartPos
    result = semExplicitGenericRoutineInst(c, dest, tree, n, routines)
  else:
    rollbackTo c.p, tmp, StartPos
    buildReadAccessor(c, tmp, tree, n, "[]")
    result = sem(c, dest, tmp, StartPos, preferredType)

proc buildWriteAccessor(c: var CheckContext; dest: var Tree; tree: Tree; n: NodePos, acc: string) =
  copyIntoKind(dest, Call, n.info):
    let accessor = c.m.strings.getOrIncl(acc)
    buildSymChoice(c, dest, accessor, n.info)
    let (le, ri) = sons2(tree, n)
    # flatten the 'BracketExpr' away:
    for ch1 in sonsReadonly(tree, le):
      copyTree(dest, tree, ch1)
    # but not the 'value' of 'a[i] = value':
    copyTree(dest, tree, ri)


  of AsgnS:
    case n.firstSon.kind
    of BracketExpr:
      var tmp = createTempTree(c.thisModule)
      buildWriteAccessor(c, tmp, tree, n, "[]=")
      result = sem(c, dest, tmp, StartPos, preferredType)
    of CurlyExpr:
      var tmp = createTempTree(c.thisModule)
      buildWriteAccessor(c, tmp, tree, n, "{}=")
      result = sem(c, dest, tmp, StartPos, preferredType)
    else:
      semAsgn(c, dest, tree, n)

@Araq
Copy link
Member

Araq commented Dec 12, 2024

And then the system.nim adds [] and []= etc for arrays which are mapped to the magic: mArrPut etc. This is slightly simpler and avoids further problems if [] is used with .borrow and distinct.

@metagn
Copy link
Collaborator Author

metagn commented Dec 12, 2024

This is ported from the old compiler, I looked into moving [] into overloads with it and there were some incompatibilities as outlined here but nothing that prevents it from working. Tuples would still have to be special cased I'm guessing.

So AtX would be the parsed expression tag, and then gets semchecked into ArrGetX/ArrPutX/maybe StrGetX/StrPutX etc? Does PatX still exist then?

@Araq
Copy link
Member

Araq commented Dec 12, 2024

Alright, your approach wins then.

@metagn
Copy link
Collaborator Author

metagn commented Dec 12, 2024

I wasn't disagreeing with your approach, the incompatibilities are fine imo (namely arbitrary pointer dereference & lax index int type) and it's cleaner. The backends will probably still need the different magics (except maybe the assign ones), NIFC wouldn't correctly handle AtX with a string operand. Another option would be to change the parsed tag to something other than AtX.

@Araq
Copy link
Member

Araq commented Dec 12, 2024

NIFC wouldn't correctly handle AtX with a string operand.

Sure but it's not its job anyway. The lowerer translates it to (at (dot stringObject s +0) i) or something like that, you get the idea.

Another option would be to change the parsed tag to something other than AtX.

Well we need more than just AtX, AtX is nifler's way of saying "a[i]", we should have our specialized variants indeed.

@Araq
Copy link
Member

Araq commented Dec 16, 2024

It would be sad to lose this so we should move it to a semcompat.nim file that will do things the Nim v2 way. Prepass for generics & templates, etc yada yada.

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.

2 participants