M-Labs
/
nac3-spec
11
0
Fork 0
Code Issues 14 Pull Requests Releases Wiki Activity

Added recent discussions #16

Merged
sb10q merged 3 commits from pca into master 2021-06-15 10:00:44 +08:00
Conversation 5 Commits 3 Files Changed 1
pca006132 commented 2021-06-09 10:01:32 +08:00

Closes #9, #12, #15.

Closes #9, #12, #15.
pca006132 added 1 commit 2021-06-09 10:01:32 +08:00
pca006132 requested review from sb10q 2021-06-09 10:02:08 +08:00
pca006132 requested review from lriesebos 2021-06-09 10:02:08 +08:00
lriesebos commented 2021-06-09 17:22:29 +08:00
  1. pseudocomments are on the same line to which they apply, not before. shown for example in this comment
  2. I added a comment to #1 because I have some concerns about this automatically populating attributes for devices based on the device db.
1. pseudocomments are on the same line to which they apply, not before. shown for example in [this comment](https://git.m-labs.hk/M-Labs/nac3-spec/issues/9#issuecomment-2105) 1. I added a comment to #1 because I have some concerns about this automatically populating attributes for devices based on the device db.
pca006132 commented 2021-06-09 17:27:10 +08:00
Poster
  1. pseudocomments are on the same line to which they apply, not before. shown for example in this comment

I'm not sure if it would be a good idea to keep them on the same line. I think some people would want a length limit on their code (e.g. 80 chars), and if the name/type name is long, the pseudocomment might have to be placed on the next line which is weird. I think it would be better to place it on top, but I'm open to changing this if you like.

  1. I added a comment to #1 because I have some concerns about this automatically populating attributes for devices based on the device db.

OK we can drop this for now and clarify it later.

> 1. pseudocomments are on the same line to which they apply, not before. shown for example in [this comment](https://git.m-labs.hk/M-Labs/nac3-spec/issues/9#issuecomment-2105) I'm not sure if it would be a good idea to keep them on the same line. I think some people would want a length limit on their code (e.g. 80 chars), and if the name/type name is long, the pseudocomment might have to be placed on the next line which is weird. I think it would be better to place it on top, but I'm open to changing this if you like. > 1. I added a comment to #1 because I have some concerns about this automatically populating attributes for devices based on the device db. OK we can drop this for now and clarify it later.
pca006132 force-pushed pca from e872af6b7d to 3f55dea87b 2021-06-09 17:28:19 +08:00 Compare
sb10q commented 2021-06-09 17:29:38 +08:00
Owner

I think it would be better to place it on top, but I'm open to changing this if you like.

Yes, that would work similarly to Rust attributes (e.g. #[cfg(feature = "target_kasli_soc")])

> I think it would be better to place it on top, but I'm open to changing this if you like. Yes, that would work similarly to Rust attributes (e.g. ``#[cfg(feature = "target_kasli_soc")]``)
lriesebos commented 2021-06-09 17:35:14 +08:00

ok, I am fine with that style of pseudocomments, it would need to be clearly defined in the manual.

ok, I am fine with that style of pseudocomments, it would need to be clearly defined in the manual.
pca006132 commented 2021-06-10 10:14:05 +08:00
Poster

So is there any changes required for this PR?

So is there any changes required for this PR?
sb10q reviewed 2021-06-10 17:40:32 +08:00
README.md Outdated
@ -50,0 +52,4 @@
```python
class Foo:
# nac3:no_warn_unused
a: int
sb10q commented 2021-06-10 17:40:32 +08:00
Owner

We can probably also support:

a: int  # nac3:no_warn_unused

The rule would be that the comment applies to the next line if the comment is the first thing on the line, and it applies to the current line if there is something before it.

We can probably also support: ``` a: int # nac3:no_warn_unused ``` The rule would be that the comment applies to the next line if the comment is the first thing on the line, and it applies to the current line if there is something before it.
👍 1
pca006132 marked this conversation as resolved
pca006132 force-pushed pca from 3f55dea87b to d85db80385 2021-06-11 09:59:22 +08:00 Compare
sb10q reviewed 2021-06-11 12:48:59 +08:00
README.md Outdated
@ -50,0 +58,4 @@
def __init__(self):
pass
```
* Instance variables that are not defined in the constructor but used elsewhere
sb10q commented 2021-06-11 12:48:59 +08:00
Owner

What if they are created on the host?

What if they are created on the host?
pca006132 commented 2021-06-11 13:03:40 +08:00
Poster

One way would be to check if the value is used, and if the constructor defines it. If not, see whether the result of executing build would assign to it.

Another direction would be to make build a static method with signature () -> Self, and pass the parameters to the constructor. IMO this is a more principled approach. Example:

class Foo(EnvExperiment):
    a: int
    def __init__(self, a: int):
        self.a = a
        
    # static, would not get self as argument
    def build():
        return Foo(1)
One way would be to check if the value is used, and if the constructor defines it. If not, see whether the result of executing `build` would assign to it. Another direction would be to make `build` a static method with signature `() -> Self`, and pass the parameters to the constructor. IMO this is a more principled approach. Example: ```python class Foo(EnvExperiment): a: int def __init__(self, a: int): self.a = a # static, would not get self as argument def build(): return Foo(1) ```
sb10q commented 2021-06-11 13:49:07 +08:00
Owner

One way would be to check if the value is used, and if the constructor defines it. If not, see whether the result of executing build would assign to it.

No, again I would not put ARTIQ-specific features into the core of the compiler.
And build() in ARTIQ is called by the constructor anyway. https://github.com/m-labs/artiq/blob/master/artiq/language/environment.py#L244

> One way would be to check if the value is used, and if the constructor defines it. If not, see whether the result of executing build would assign to it. No, again I would not put ARTIQ-specific features into the core of the compiler. And build() in ARTIQ is called by the constructor anyway. https://github.com/m-labs/artiq/blob/master/artiq/language/environment.py#L244
pca006132 commented 2021-06-11 13:54:20 +08:00
Poster

The problem is that we cannot really analyze host code statically, we can only run it. What about the second option?

The problem is that we cannot really analyze host code statically, we can only run it. What about the second option?
pca006132 commented 2021-06-11 13:59:46 +08:00
Poster

I'm mainly thinking about static analyze without actually executing the constructor. We can also let the host execute the constructor and then check for field definition when we create the kernel object (and in fact we must do so if the kernel is host only). Should I document this?

I'm mainly thinking about static analyze without actually executing the constructor. We can also let the host execute the constructor and then check for field definition when we create the kernel object (and in fact we must do so if the kernel is host only). Should I document this?
sb10q commented 2021-06-11 14:09:44 +08:00
Owner

We can also let the host execute the constructor and then check for field definition when we create the kernel object

Yes, that sounds like the only viable option in my opinion.

Should I document this?

I think so. And also explain the situation with objects that are created in kernels (with @kernel or @portable on __init__).

> We can also let the host execute the constructor and then check for field definition when we create the kernel object Yes, that sounds like the only viable option in my opinion. > Should I document this? I think so. And also explain the situation with objects that are created in kernels (with ``@kernel`` or ``@portable`` on ``__init__``).
pca006132 marked this conversation as resolved
sb10q reviewed 2021-06-11 12:50:57 +08:00
README.md Outdated
@ -144,0 +159,4 @@
mode checking (recursively check all elements, which would be slower for
large lists) or performance mode which only check the first element of each
list. (#15)
* Code region protected by a type check, such as `if is_type(x, int):`, would
sb10q commented 2021-06-11 12:50:57 +08:00
Owner

Mention that is_type is the only way to do type checks, i.e. isinstance(...) and type(...) == ... do not compile.

Mention that ``is_type`` is the only way to do type checks, i.e. ``isinstance(...)`` and ``type(...) == ...`` do not compile.
pca006132 marked this conversation as resolved
pca006132 force-pushed pca from d85db80385 to c5769686f2 2021-06-11 13:05:27 +08:00 Compare
pca006132 force-pushed pca from c5769686f2 to 13591c5999 2021-06-11 14:32:38 +08:00 Compare
sb10q reviewed 2021-06-12 07:02:04 +08:00
README.md Outdated
@ -50,0 +59,4 @@
pass
```
* Use-before-define:
* Host-only constructor/contains `build` method: Warn if a certain field `f` is
sb10q commented 2021-06-12 07:02:04 +08:00
Owner

The build() method is ARTIQ-specific and just something that is called by the EnvExperiment constructor, which may or may not be used to build compilable objects. When it is, we are simply in the "object fields were defined by host" case.
Special handling of the build() method has no place in the compiler.

The ``build()`` method is ARTIQ-specific and just something that is called by the ``EnvExperiment`` constructor, which may or may not be used to build compilable objects. When it is, we are simply in the "object fields were defined by host" case. Special handling of the ``build()`` method has no place in the compiler.
pca006132 marked this conversation as resolved
sb10q reviewed 2021-06-12 07:03:06 +08:00
README.md Outdated
@ -50,0 +62,4 @@
* Host-only constructor/contains `build` method: Warn if a certain field `f` is
used in other kernel methods but missing from the object constructed in
the host.
* `@portable`/`@kernel` constructor with no `build` method: Warn if a certain
sb10q commented 2021-06-12 07:03:06 +08:00
Owner

Shouldn't it error out and not just warn? Calling any method on uninitialized memory sounds like certain trouble.

Shouldn't it error out and not just warn? Calling any method on uninitialized memory sounds like certain trouble.
pca006132 marked this conversation as resolved
pca006132 force-pushed pca from 13591c5999 to b79c1725b3 2021-06-15 09:56:30 +08:00 Compare
sb10q merged commit b79c1725b3 into master 2021-06-15 10:00:44 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
sb10q
lriesebos
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/nac3-spec#16
There is no content yet.
Delete Branch "pca"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?