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

Clean up ast.TIdTable type definition #145

Open
haxscramper opened this issue Dec 31, 2021 · 3 comments
Open

Clean up ast.TIdTable type definition #145

haxscramper opened this issue Dec 31, 2021 · 3 comments
Labels
good first issue Easy-to-start-with issue with no in-depth knowledge or complex implementation required. refactor Implementation refactor

Comments

@haxscramper
Copy link
Collaborator

ast defines TIdTable type that is used in several parts of the semantic pass. Type defined as follows:

  TIdPair* = object
    key*: PIdObj
    val*: RootRef

  TIdPairSeq* = seq[TIdPair]
  TIdTable* = object # the same as table[PIdent] of PObject
    counter*: int
    data*: TIdPairSeq

All the uses of the val: RootRef field are either converted to PType or PSym (result = PType(idTableGet(c.bindings, t)) etc.). This means RootRef can be easily replaced with

type
  PSymOrTyp = object
    case isSym*: bool
      of true:
        sym*: PSym

      of false:
        typ*: PType

This would not create any functional difference, but will clean up the code a little.

@haxscramper haxscramper added good first issue Easy-to-start-with issue with no in-depth knowledge or complex implementation required. refactor Implementation refactor labels Dec 31, 2021
@alaviss
Copy link
Contributor

alaviss commented Dec 31, 2021

This will make it a touch too expensive. Right now the field takes 8 bytes on a 64-bit machine. If we were to add a bool sentinel, alignment will cause this to inflate to 16 bytes, which doubles the memory usage...

As much as I hate it, exploiting inheritance works better for this instance.

@alaviss
Copy link
Contributor

alaviss commented Dec 31, 2021

We can tighten it up by making val to be PIdObj as well, as that is the common ancestor of PType and PSym.

@haxscramper
Copy link
Collaborator Author

All uses of the idTableGet are either PSym -> PSym or PType -> PType, which means we can make this procedure a generic, with the PIdObj for val

haxscramper added a commit to haxscramper/nimskull that referenced this issue May 29, 2022
haxscramper added a commit to haxscramper/nimskull that referenced this issue Jun 14, 2022
haxscramper added a commit to haxscramper/nimskull that referenced this issue Jul 30, 2022
haxscramper added a commit to haxscramper/nimskull that referenced this issue Jul 30, 2022
commit 16d360f
Author: haxscramper <haxscramper@gmail.com>
Date:   Sun Jun 19 17:10:35 2022 +0300

    diff tuple structure in the type formatting

commit 67f07d7
Author: haxscramper <haxscramper@gmail.com>
Date:   Sun Jun 19 16:21:47 2022 +0300

    allow to show formatted fields controls for astrepr

commit 948fd51
Author: haxscramper <haxscramper@gmail.com>
Date:   Sun Jun 19 15:46:57 2022 +0300

    show value in the invalid enum message

commit d810886
Author: haxscramper <haxscramper@gmail.com>
Date:   Sun Jun 19 15:08:14 2022 +0300

    side effect annotations message

commit 42ff1ac
Author: haxscramper <haxscramper@gmail.com>
Date:   Tue Jun 14 23:05:03 2022 +0300

    use `getIdentStr()`

    Spent half an hour trying to figure out why the implementation does not
    work properly, only to realize `getStr()` raises 'recoverable'
    'error' (`ERecoverableError`), which (IMO) are a mutually exclusive
    words, but what do I know. Anyway, because `getStr()` had been defined
    for strings, it naturally failed to work with regular identifiers, and
    promptly failed when I tried to figure out the name of the field. But
    because error was 'recoverable' it bounced off some part of the
    execution trace, and got swallowed somewhere.

    This is a good example of how simple misuse of the code, paired with
    'recoverable' error approach can cause issues that are almost impossible
    to debug cleanly, since there are little to no observable side effects.
    All failure handling in the compiler must be explicit, and 80-or-so
    calls to the `globalError` should be eradicated in the future, because
    each of them might silently throw you in completely different part of
    the compiler code.

commit 938a4bb
Author: haxscramper <haxscramper@gmail.com>
Date:   Tue Jun 14 22:22:29 2022 +0300

    when debug utils is used, trigger `undef` early

    - when `undef()` is used in compiler with debugutils enabled, check for
      certain symbols early to avoid messing up the semantic checking
      trail. `undef(nimDebugUtils)` not cuts off cleanly at the end, instead
      of unconditionally placing `semExpr` & related procs
    - make all astrepr functions `noSideEffect` using cast

commit 4e1ca71
Author: haxscramper <haxscramper@gmail.com>
Date:   Tue Jun 14 21:34:34 2022 +0300

    more documentation for debugging

commit 26a9dfa
Author: haxscramper <haxscramper@gmail.com>
Date:   Tue Jun 14 23:11:57 2022 +0300

    undeclared field name error message

commit 1fab406
Author: haxscramper <haxscramper@gmail.com>
Date:   Mon Jun 13 19:49:30 2022 +0300

    format undeclared identifier error message

commit 9134bd7
Author: haxscramper <haxscramper@gmail.com>
Date:   Sun May 29 12:26:59 2022 +0300

    formatting changes

commit 980a854
Author: haxscramper <haxscramper@gmail.com>
Date:   Sun May 29 12:18:05 2022 +0300

    show generic type parameter substitution

    generic type parameters /were/ stored in TIdTable, but I had to filter
    out elements that are `of PType`.

commit 3115a5b
Author: haxscramper <haxscramper@gmail.com>
Date:   Sun May 29 11:44:34 2022 +0300

    store instantiation parameters in the info context

    I can't figure out how to get the data necessary to generate message
    like "instantiation of `procedure` with `A = int, B = float`" (list
    supplied generic parameters).

commit c21968a
Author: haxscramper <haxscramper@gmail.com>
Date:   Sun May 29 10:14:58 2022 +0300

    Replace `RootRef` -> `PIdObj`

    nim-works#145

commit 5144eb4
Author: haxscramper <haxscramper@gmail.com>
Date:   Sat May 28 20:56:29 2022 +0300

    basic type mismatch message ranking implemented

commit 68340f1
Author: haxscramper <haxscramper@gmail.com>
Date:   Sat May 28 19:58:52 2022 +0300

    initial formatter implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Easy-to-start-with issue with no in-depth knowledge or complex implementation required. refactor Implementation refactor
Projects
None yet
Development

No branches or pull requests

2 participants