google / globalfoundries-pdk-libs-gf180mcu_fd_pv

Apache License 2.0
12 stars 6 forks source link

set unused variable to nil after .forget #53

Open proppy opened 1 year ago

proppy commented 1 year ago

https://www.klayout.de/doc/about/drc_ref_layer.html#h2-1260 recommends the following pattern after .forget'ing a variable:

l = ... # compute some layer
...
# once you're done with l:
l.forget
l = nil

we should make sure we follow it.

atorkmabrains commented 1 year ago

@proppy klayout already sets the layer to nil when you call forget. And I believe we have demonstrated this in one of the PRs. This is just redundancy which I think is not necessary.

Here is the link to klayout forget code: https://github.com/KLayout/klayout/blob/c931a08ec08bb626b28d3009e015a0964936b012/src/drc/drc/built-in-macros/_drc_layer.rb#L265

proppy commented 1 year ago

Then maybe we should submit a PR to klayout docstring https://github.com/KLayout/klayout/blob/c931a08ec08bb626b28d3009e015a0964936b012/src/drc/drc/built-in-macros/_drc_layer.rb#L253-L263 to stop advertising this as a best practice?

/fyi @klayoutmatthias

atorkmabrains commented 1 year ago

@proppy I think you might need to open that on Klayout. As I think forget method actually set the layer to nil as we have discussed.

klayoutmatthias commented 1 year ago

That is easy to change :) ... my reasoning actually was that without setting the layer to nil, you will find the layer variable still holding a valid object which however is dead. Internally of course the expensive payload object is set to nil already. I thought that might be considered as a kind of violation of basic OO contracts. But it's a matter of taste.

Ideally, setting the variable simply to nil would make the object forget, but Ruby is not like that because GC may be delayed.

atorkmabrains commented 1 year ago

Thanks @klayoutmatthias

@proppy I believe we can close this at our end. Isn't that correct?