[superseded] move comment field out of TNode => sizeof(TNode) = 32 instead of 40

[timotheecour]

• this WIP PR fixes the main issue raised in https://github.com/nim-lang/Nim/issues/10046 :

A NimNode (PNode in the compiler) has a cramped lineinfo with only a few bits for line and column information. People already hit it's limits. On the other hand, there is a comment field in every node that doesn't contain information at all. Most of the time it is unused.

after this PR, sizeof(TNode) = 32 instead of 40.

• it would allow exposing comment to macros.nim (via a revived version of [TODO] add macros.comment(NimNode) magic to get node doc comment #8903)

here's the main change:

comment is only placed where it's useful: in a node of kind nkCommentStmt, or in a PSym (where we're not as concerned about space since there are fewer such PSym compared to TNode; TSym is already quite large and adding a comment field to it doesn't add up to much )

 TNode*{.final, acyclic.} = object # on a 32bit machine, this takes 32 bytes
    when defined(useNodeIds):
      id*: int
    typ*: PType
    info*: TLineInfo
    flags*: TNodeFlags
    case kind*: TNodeKind
    of nkCharLit..nkUInt64Lit:
      intVal*: BiggestInt
   ...
    of nkSym:
      sym*: PSym # PSym has a new `commentSym` field
    of nkIdent:
      ident*: PIdent
    of nkCommentStmt:
      commentStmt*: string # this is new
    else:
      sons*: TNodeSeq
   # `comment` was removed! 

current results (WIP)

I was able to recompile nim and run code as well as nim doc on this, here's current results:
it's lacking comments except for runnableExamples and mainFun currently

#[
KEY xyz
]#

const bar* = 1 ## c1
  ## sdfasf

type Baz* = enum ## asdf
  d1, ## asdf
  d2, ## asdf

type Foo* = object ## casdf
  a1: int ## c1
  a2: int ## c2

proc mainFun*()=
  ## c2
  ## c3
  runnableExamples:
    ## c4
    discard

  echo "ok1"

proc main*()=
  ## c2
  ## c3
  echo "ok1"

main()

it renders as:

what I'm not yet sure about

• My logic requires a node of kind nkCommentStmt doesn't have sons, however it seems used in following code (where I had to comment out the recorded.add since sons isn't available anymore):

proc recordPragma(c: PContext; n: PNode; key, val: string; val2 = "") =
  var recorded = newNodeI(nkCommentStmt, n.info)
  when false: # TODO
    recorded.add newStrNode(key, n.info)
    recorded.add newStrNode(val, n.info)
    if val2.len > 0: recorded.add newStrNode(val2, n.info)
  c.graph.recordStmt(c.graph, c.module, recorded)

It seems to me compiler could be patched, that recordPragma could be done differently to allow what I'm doing here

• need to think about how to handle rawSkipComment

[timotheecour]

ok after some more work i get this (see below).
I've added commentIdent*: string to TIdent (which, IIUC, isn't a space bottleneck since it's less numerous)

[Araq]

@timotheecour er, @krux02 is actively working on this feature too.

[timotheecour]

mainly did this as an experiment to see whether we could get away with removing that comment field (to address space concern) without introducing breaking changes that would arise from modifying AST (the sons field), ie as an alternative approach to #10046.

@krux02 @Araq how will #10046 avoid code breakage, eg for macros that use lastSon? (it would now get a different node if a doc comment node is added at the end)
or variations of it, eg, n[^1] etc

[krux02]

The node kind is different now. I don't think there is any code that accesses a nnkPostfix node without checking before, that it is actually a postfix node. This check will either fail and has to be extended for the now node kind.

Since I am actively working on this issue I am closing your PR here. I hope you are OK with it.

[timotheecour]

Since I am actively working on this issue I am closing your PR here. I hope you are OK with it.

I'm ok with you closing my PR if you're actively working on this issue but last commit on #10061 was 17 days ago (819dd27) ;
I understand it's not an easy task, just making sure it wasn't abandonned (not being pushy, genuine question).
Also, your PR #10061 and the issue https://github.com/nim-lang/Nim/issues/10046 are marked as locked so can't accept any new comments, I really don't see locking as helping open source (and I haven't seen locking being used in Nim before)

[EDIT] upon further investigation, it looks like you've blocked my account from commenting on any issue/PR that was authored by you (I'm hoping it was by accident, please unblock me as this makes no sense) see https://irclogs.nim-lang.org/07-01-2019.html#05:06:20
; For example, I was going to comment on #10185 that #10218 fixes that issue for static int case, but I can't because of that block.
/cc @krux02

[krux02]

just an update. I am actively working on this change. But you know, there was christmas. I didn't work during that time. Since this is a breaking change it takes a while, too. I want it to be the least amount of breaking as possible. And everything that used to compile should have a very nice error message. I don't want to break somebody's code with a nice error message such as "internal error".

[krux02]

Since my work on this issue is on ice I reopen this.

[narimiran]

just making sure it wasn't abandoned (not being pushy, genuine question).

@timotheecour this has "work in progress" in the title: has there been any work or progress recently? Can we close this one, and you can open a new one when the merge conflicts are resolved and more work has been done (if there's still a need for it)?

[Araq]

PR has stalled, feel free to rebase and reopen, removing the comment field from the AST would still be much appreciated.

[timotheecour]

superseded by #18760