cosunae
commented
6 years ago thanks for the feedback @clementval , some comments from my side:
- concerning the named elements, I guess we have two ways to reference children, by name (which is the current proposal) or by position (which is what you suggest?). I see your point that having to specify a name sometimes makes the information redundant. I think the main use case for it was to easily group an array (of dimensions for example). So if we go to "by position" I guess we will need an Array element?
- The literal was intended to be used only within an arithmetic expression, so do we need string? Most other places where strings are needed are things like "name" of a field, and for that we are using attributes. So do we need a string literal?
- Binary op: not sure I understand, you would remove the BinaryOp and create a bunch of other elements, one for each operation? This will considerably increase the number of elements...
- VarAccess can not do i+1, since it can dereference only scalar (not gridded fields). Something like
double a[4]; ->VarDecl a[2] = 0.0; ->VarAccessThis was intentional, since for
i+1we should use FieldDecl - GridDimension, uhmmm we can identify by index (Literal+) or by name, so currently we have a name in the attributes ?
- Program: yes, could you update it in the PR?
clementval
This is a placeholder for my feedback/review. If any changes are simple, I'll directly issue a PR.
1) - [x] named element: As a general comment, I would remove the notion of named elements as it appears now. I think this is quite confusing and it would be more straightforward to have the element with the correct name directly. The elements can then share their internal specification but it would be clear from the beginning. This is the case for example of
lhs/rhs. This would remove also the ambiguity with the actual name (in fact identifier) of variables in the IR. Another example can be found in the example. Thevarargname forVarDeclis used directly as element name can imply directly the attributeisarg=true.2) - [x] id instead of name: Following the previous comment, I would change the
nameinVarDecland other element representing the variable inidwhich is more representative of what the information in the IR is.3) - [x] Literal: Wouldn't it make sense to have the differentiation between String literal and integer/real constant? How do you make the difference otherwise between a string
"1.0"and an actual real1.0?4) - [x] binary op: Similar to named elements, I would directly have the operation as the name of the element and not as an attribute.
DivOp,MulOp,AddOp,SubOp,PowerOp... Doesn^t matter then if they are all derived from a singleBinaryOp. This would make the IR more readable.5) - [x] VarAccess: How can we access a var with
i+1for example. The current specification acceptsLiteralelements only. I guess it should accept a restricted set ofExprModel.6) - [x] GridDimension: I think this element is not well defined. It as no content model but should have something like
(Literal+)or(Var+)...7) - [x] Program: The content model is referencing
Dimensionbut should beGridDimension. Can be related with 1.WORK IN PROGRESS...
Open PRs: Removal of
StmtandExprStmtas elements #1