feat: allow VAR_EXTERNAL declarations
#1324
Conversation
mhasel
changed the title
VAR_EXTERNAL blocksVAR_EXTERNAL declarations
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1324 +/- ##
==========================================
+ Coverage 91.11% 91.42% +0.30%
==========================================
Files 153 160 +7
Lines 44816 49497 +4681
==========================================
+ Hits 40833 45251 +4418
- Misses 3983 4246 +263 ☔ View full report in Codecov by Sentry. |
| @@ -70,6 +70,8 @@ pub struct VariableIndexEntry { | |||
| pub argument_type: ArgumentType, | |||
| /// true if this variable is a compile-time-constant | |||
| is_constant: bool, | |||
| // true if this variable is in a 'VAR_EXTERNAL' block | |||
| is_var_external: bool, | |||
There was a problem hiding this comment.
| is_var_external: bool, | |
| is_external: bool, |
There was a problem hiding this comment.
I've considered naming it is_external too but then decided against it since it might be easy to mix-up with {external} linkage
There was a problem hiding this comment.
Yeah same here, I just thought for consistency sake (e.g. is_constant) we'd want it to be also called is_external, also related https://github.com/PLC-lang/rusty/pull/1324/files/0e46463c17a2ee1b0b055757beaf855c2e8586bf#r1791423510
But I'm fine either way, whichever one you prefer
| @@ -253,6 +263,10 @@ impl VariableIndexEntry { | |||
| self.linkage == LinkageType::External | |||
| } | |||
|
|
|||
| pub fn is_var_external(&self) -> bool { | |||
There was a problem hiding this comment.
| pub fn is_var_external(&self) -> bool { | |
| pub fn is_external(&self) -> bool { |
There was a problem hiding this comment.
and maybe rename the previous is_external function to is_external_linkage then?
There was a problem hiding this comment.
Honestly I'd prefer to keep the external linkage function as is_external since, at least in my mind, if something says it is external I'd assume linkage. This might make the code harder to follow when encountering is_external somewhere and it refers to VAR_EXTERNAL variables. We can rename the is_var_external to something else if you have a suggestion, but I think is_external should continue to refer to LinkageType::External.
@ghaith any thoughts?
There was a problem hiding this comment.
Yeah maybe you're right, especially since VAR_EXTERNALs are not supported anyways
| Variable { | ||
| name: "arr", | ||
| data_type: DataTypeDefinition { | ||
| data_type: ArrayType { | ||
| name: None, | ||
| bounds: RangeStatement { | ||
| start: LiteralInteger { | ||
| value: 0, | ||
| }, | ||
| end: LiteralInteger { | ||
| value: 100, | ||
| }, | ||
| }, | ||
| referenced_type: DataTypeReference { | ||
| referenced_type: "INT", | ||
| }, | ||
| is_variable_length: false, | ||
| }, | ||
| }, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Does this have to be a pointer to the actual global variable or are external variables imported as locals by deep-cloning them? Just a thought, doesn't change anything in this PR since we don't support them anyways.
There was a problem hiding this comment.
They are parsed like regular variables at the moment. Afaik they don't need to be pointers, this info would just be used when validating that the variable exists as a global and has matching types etc.
Co-authored-by: Volkan <[email protected]>
Adds a new variable block type for
VAR_EXTERNALto the compiler. These blocks do not have any functionality for now and the compiler will emit a warning when such blocks are declared.