feat: adds cpp/def - #762
Conversation
10f5a2f to
384fb7e
Compare
c82094d to
1513bcc
Compare
jeaye
left a comment
There was a problem hiding this comment.
We're getting there, Saket!
I have done some of the thought work to seed the testing of this for you. You need to do more of it yourself, though. It's a very important exercise. If you're unsure of anything, ask me.
| @@ -0,0 +1,6 @@ | |||
| (ns jank.cpp.def.fail-clash-with-other-cpp-def) | |||
|
|
|||
| (cpp/def int foo 5) | |||
There was a problem hiding this comment.
We need to think about REPL-based usage cases. jank is not just an AOT compiled language. Someone may eval a buffer containing a cpp/def and we need to handle that gracefully.
- If
foohas already been defined by acpp/def, we need to check the type to ensure it's being defined with the same type. If so, the effect of thecpp/defshould be to assign the new value, not to define the variable again - If the type is different, we need to raise a compilation error (dedicated error for this)
However, if we're generating assignments for pre-defined symbols, that type actually needs to be assignable, which leads us to:
- Const types will not allow assignment
- Types with deleted assignment operators will not allow assignment
In these cases, we need to raise a compiler error since we can't re-evaluate the cpp/def in any meaningful way.
Also, we need to handle the cases of tricky construction. For example:
- No value is provided, but the type is not default constructible
- A value is provided, but the type is not copy constructible (but is move constructible)
- A value is provided, but the type is neither move nor copy constructible
- A value is provided, but no matching constructor is found
On top of that, we need to exercise the analysis more, with different functional tests. For example:
- Missing type form
- Missing name + value form
- Multiple values are provided (extra forms)
- The name symbol is qualified
- The name is not a symbol
Also, all of our values passed in are currently using trait conversion. We have not covered either of these cases:
- The type is a typed or type-erased jank object
- The type native and the value is actually native, too
- The type is native and the value is implicitly convertible (not trait convertible)
Each of these should be covered, but the list is not exhaustive. Consider how this feature will be used and how it can break. Especially with these functional tests, no detailed C++ knowledge is required. Just thinking about the different forms that can be passed in will lead you to failure cases.
|
|
||
| (cpp/def int foo "bar") | ||
|
|
||
| :success |
There was a problem hiding this comment.
This test and compiler+runtime/test/jank/cpp/def/fail-type-mismatch.jank are doing the same htin.
Neither of them is actually a compiler error. If you want a type mismatch, use native types!
| @@ -0,0 +1,9 @@ | |||
| (ns jank.cpp.def.pass-multiple-defs) | |||
There was a problem hiding this comment.
Each test runs in the user ns by default. We don't want to end up in a different ns because of a prior test. So, we either need to:
- Return to the
userns at the end of these tests - Just stay in the
userns
I'd prefer the latter. Instead of using foo, use a symbol unique to the test and just stay in the user ns.
| @@ -0,0 +1,10 @@ | |||
| (ns jank.cpp.def.pass-with-instantiation) | |||
|
|
|||
| (cpp/raw "struct Point { int x; int y; Point(int, int); Point* scale(int); void show(); }; ") | |||
There was a problem hiding this comment.
This needs to be in a C++ namespace which matches the test. Refer to other cpp/raw usages in this test suite for examples.
| (ns jank.cpp.def.pass-with-instantiation) | ||
|
|
||
| (cpp/raw "struct Point { int x; int y; Point(int, int); Point* scale(int); void show(); }; ") | ||
| (cpp/raw "Point::Point(int x, int y): x{ x }, y{ y }{}") |
There was a problem hiding this comment.
Why is this defined separately? Also, why is any constructor provided at all? We should just be able to use aggregate initialization.
| (if (and (= (+ my-number 1) 6) | ||
| (= my-vec [1 2])) | ||
| :success | ||
| (throw :failure)) |
There was a problem hiding this comment.
Do not throw, or return :failure or anything else in the failure case. Anything other than :success is already a failure. The extra code is just more to worry about.
570cdfc to
11d8eee
Compare
No description provided.