-
Notifications
You must be signed in to change notification settings - Fork 164
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
ASR: change how a symbol is printed #1510
Comments
I am not sure we need both If we need both, I prefer If we can get away with one of the two, why not do that? |
Clojure will interpret |
We can rename |
let's discuss this in person. My sense of clarity is decreasing, not
increasing :)
…On Mon, Feb 6, 2023 at 9:15 PM Ondřej Čertík ***@***.***> wrote:
symbol is currently used in quite a few nodes. If we want to use Var, we
need to replace symbol with expr, and enforce in verify() that the expr
is an actual Var and no other expr. This is a limitation of ASDL.
We can rename Var to SymbolRef, but that I think does not change anything
in the structure and by itself does not solve anything.
—
Reply to this email directly, view it on GitHub
<#1510 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABSRR4GRB3T2SPM2IWJJJDWWHLALANCNFSM6AAAAAAUTKKK34>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Let me try for a little more clarity with some point-like questions:
|
Here is an example illustrating ambiguity with naked streams. These are two FunctionCalls from tests/expr7.py: ;; (FunctionCall
;; 1 \____ should be (SymbolRef 1 test_pow_1)
;; test_pow_1 /
;; () >---- should be NullSymbolRef (new symconst)
;; [((IntegerConstant 1 (Integer 4 []))) ; call_args
;; ((IntegerConstant 2 (Integer 4 [])))]
;; (Integer 4 [])
;; ()
;; ())
;; (FunctionCall _____________________
;; 2 \____ should be SymbolRef(...)
;; pow/__lpython_overloaded_0__pow __/
;; 2 \____ should be (SymbolRef 2 pow)
;; pow __/
;; [((IntegerConstant 2 (Integer 4 [])))
;; ((IntegerConstant 2 (Integer 4 [])))]
;; (Real 8 [])
;; (RealConstant 4.0 (Real 8 []))
;; ()) The (tricky, non-robust) code for processing (working-around) these examples is around line 476 in rebcabin/ClojureProjects002@2b92b43#diff-1a0a9fc9fb7f9291d27c713a3b60f54e75050e723a463da5dd9df82a20a68080R456 |
Hey, is this issue available? Can I work on this issue? |
?? |
@aadityasinha-dotcom you can work on any issue you want, simply submit a PR. |
Currently a
symbol
(e.g. inVar(symbol v)
) is printed as2 x
, meaning it's the symbol table ID 2, variable "x" from that table. For example,(Var 2 x)
.This has issues with parsing where you have to parse two items for
symbol
instead of just one, and also things look inconsistent, is it a symbol (owning) or a pointer to a symbol (non-owning)?The best proposed idea how to fix this is to introduce:
And then the current
(Var 2 x)
becomes(Var (SymbolRef 2 x))
.See #1492 (comment) for more details.
This cleans up the abstract representation of ASR, now
symbol
is always owning, andSymbolRef
is always non-owning.Important: we need to preserve the current speed, so in C++ the
SymbolRef
would actually always be replaced with just a pointer to thesymbol
, so the generated C++ code would be equivalent to what it is today, so no slow down. But conceptually, the pointer to symbolx
is the same as(SymbolRef 2 x)
. In languages that do not have pointers, such while printing or serializing (or Clojure), one can use(SymbolRef 2 x)
directly.Other ideas, just for printing:
(Var (2 x))
(Var {2 x})
But it seems our experience shows that it's better to use longer, descriptive names in ASR printouts and not to use shortcuts, so that it's more obvious what the printout represents to newcomers. The ASR Clojure like printout is NOT used for performance parts of the code. For that we use binary representation where we eventually implement all tricks possible to keep the binary representation as compact as possible, or alternatively to deserialize it as quickly as possible.
Related issues:
ExplicitDeallocate.vars
is not asymbol *
#1492The text was updated successfully, but these errors were encountered: