Clojure

Add ability to disable locals clearing

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: Release 1.4
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code
  • Approval:
    Ok

Description

When using a debugger, it is useful to be able to see all variable values. Locals clearing reduces the number of variabls that are non-nil, and makes uncertain whether nil values are real or an artifact of locals clearing.

Please add a mechanism for disabling locals clearing.

The attached patch does this by introducing the disable-locals-clearing var, and using it to initialise the canBeCleared field of LocalBinding.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Aaron Brooks added a comment -

+1

The ability to debug is greatly degraded when locals on lower stack frames have already been cleared. I'll apply this patch and will test in the next few days.

Show
Aaron Brooks added a comment - +1 The ability to debug is greatly degraded when locals on lower stack frames have already been cleared. I'll apply this patch and will test in the next few days.
Hide
Permalink
Stuart Halloway added a comment -

It seems we might have a number of new debug settings ahead. Are there other approaches besides filling the core namespace? Names in a different namespace? A single name in core that points to a map?

Show
Stuart Halloway added a comment - It seems we might have a number of new debug settings ahead. Are there other approaches besides filling the core namespace? Names in a different namespace? A single name in core that points to a map?
Hide
Permalink
Phil Hagelberg added a comment -

We could have this triggered by system properties. We've talked about having a clojure.debug system property; it would make sense to have this set a number of other properties including one specifically for locals clearing.

Show
Phil Hagelberg added a comment - We could have this triggered by system properties. We've talked about having a clojure.debug system property; it would make sense to have this set a number of other properties including one specifically for locals clearing.
Hide
Permalink
Hugo Duncan added a comment -

From a user perspective, I would like to be able to control locals clearing on a fine grained basis. In slime, for instance, C-u C-c C-c could be used to compile with locals disabled. This would improve the possibilities for debugging code that required locals clearing to work properly.

To me this would suggest the use of a var (though the var could be held in a map).

Show
Hugo Duncan added a comment - From a user perspective, I would like to be able to control locals clearing on a fine grained basis. In slime, for instance, C-u C-c C-c could be used to compile with locals disabled. This would improve the possibilities for debugging code that required locals clearing to work properly. To me this would suggest the use of a var (though the var could be held in a map).
Hide
Permalink
Kevin Downey added a comment -

clojure.settings/disable-locals-clearing sounds good to me

Show
Kevin Downey added a comment - clojure.settings/disable-locals-clearing sounds good to me
Hide
Permalink
David Santiago added a comment -

I much prefer either the clojure.settings namespace (or similar), or the map of options. I think what Hugo is describing as an important use case to keep in mind, but also, if Clojure itself has the facilities to handle this functionality, I don't see any reason to tie it to Java's property system. If compilation options proliferate, it doesn't seem unlikely to me that multiple target Clojures (CLR, CLJS) might also use similar flags, and by keeping it in Clojure it could make that simpler to deal with.

Hugo also pointed out to me privately that we could also make these vars be initialized from system properties, which seems to me like it would get a lot of the convenience you would get from having this based on system properties.

Finally, I'd just like to say that I think clojure.settings/locals-clearing, default: true sounds a little better to me. It would make code using that variable a little clearer because then the enable/disable matches the true/false value it holds.

Show
David Santiago added a comment - I much prefer either the clojure.settings namespace (or similar), or the map of options. I think what Hugo is describing as an important use case to keep in mind, but also, if Clojure itself has the facilities to handle this functionality, I don't see any reason to tie it to Java's property system. If compilation options proliferate, it doesn't seem unlikely to me that multiple target Clojures (CLR, CLJS) might also use similar flags, and by keeping it in Clojure it could make that simpler to deal with. Hugo also pointed out to me privately that we could also make these vars be initialized from system properties, which seems to me like it would get a lot of the convenience you would get from having this based on system properties. Finally, I'd just like to say that I think clojure.settings/locals-clearing, default: true sounds a little better to me. It would make code using that variable a little clearer because then the enable/disable matches the true/false value it holds.
Hide
Permalink
Stuart Sierra added a comment -

The proliferation of dynamic Vars in core has concerned me too. Java System Properties are nice for Java interop, but they can only have String values and they lack thread-local binding semantics.

A dynamic map would work, but binding it correctly is more complicated:

(binding [*settings* (merge *settings* {...})] ...)

This could be mitigated with a macro.

A namespace for settings is convenient for documentation purposes, but what about the cost of dynamic Vars? If they're only used at compile-time, it doesn't really matter, but a single Var lookup is still cheaper than many.

Show
Stuart Sierra added a comment - The proliferation of dynamic Vars in core has concerned me too. Java System Properties are nice for Java interop, but they can only have String values and they lack thread-local binding semantics. A dynamic map would work, but binding it correctly is more complicated: 
(binding [*settings* (merge *settings* {...})] ...)
This could be mitigated with a macro. A namespace for settings is convenient for documentation purposes, but what about the cost of dynamic Vars? If they're only used at compile-time, it doesn't really matter, but a single Var lookup is still cheaper than many.
Hide
Permalink
Phil Hagelberg added a comment -

Are we really concerned about thread-locality when dealing with compiler settings? Concurrent compilation already has issues with transactionality, so I got the impression that it's not encouraged. I don't see the string limitation being a problem here, but perhaps there are other use cases where it would be annoyingly limiting?

Show
Phil Hagelberg added a comment - Are we really concerned about thread-locality when dealing with compiler settings? Concurrent compilation already has issues with transactionality, so I got the impression that it's not encouraged. I don't see the string limitation being a problem here, but perhaps there are other use cases where it would be annoyingly limiting?
Hide
Permalink
Stuart Sierra added a comment - - edited

Phil wrote: Are we really concerned about thread-locality when dealing with compiler settings?

We aren't necessarily concerned with thread-locality, although the compiler itself a lot of dynamically-scoped Vars internally. For external use, all we really need is scoped, temporary settings, so Java system properties would work with some helper macrology like with-properties.

The string limitation isn't a problem right now, but it could be annoying if we added something like CL feature-test expressions.

Show
Stuart Sierra added a comment - - edited Phil wrote: Are we really concerned about thread-locality when dealing with compiler settings? We aren't necessarily concerned with thread-locality, although the compiler itself a lot of dynamically-scoped Vars internally. For external use, all we really need is scoped, temporary settings, so Java system properties would work with some helper macrology like with-properties. The string limitation isn't a problem right now, but it could be annoying if we added something like CL feature-test expressions.
Hide
Permalink
Hugo Duncan added a comment -

Inverts the logic of the previous patch and uses enable-locals-clearing.

I'm unclear as to what the consensus is on how to avoid var proliferation.

Show
Hugo Duncan added a comment - Inverts the logic of the previous patch and uses enable-locals-clearing. I'm unclear as to what the consensus is on how to avoid var proliferation.
Hide
Permalink
Hugo Duncan added a comment -

This patch implements compiler-options as a map with the :locals-clearing key.
The initialisation is via the value of the clojure.compile.locals-clearing system property.

Show
Hugo Duncan added a comment - This patch implements compiler-options as a map with the :locals-clearing key. The initialisation is via the value of the clojure.compile.locals-clearing system property.
Hide
Permalink
Hugo Duncan added a comment -

0001-add-compiler-options-map.diff

Implements compiler-options as a map, with a :locals-clearing key, initialised from the clojure.compile.locals-clearing system property.

Show
Hugo Duncan added a comment - 0001-add-compiler-options-map.diff Implements compiler-options as a map, with a :locals-clearing key, initialised from the clojure.compile.locals-clearing system property.
Hide
Permalink
Andy Fingerhut added a comment -

Just a note that all 3 patches attached right now fail to apply cleanly using 'git --keep-cr -s < patchfile.txt' to latest master as of March 9, 2012. Any consensus on which of these approaches to take? I can update that one if so.

Show
Andy Fingerhut added a comment - Just a note that all 3 patches attached right now fail to apply cleanly using 'git --keep-cr -s < patchfile.txt' to latest master as of March 9, 2012. Any consensus on which of these approaches to take? I can update that one if so.
Hide
Permalink
George Jahad added a comment -

Per RH's request, I tested this with CDT, and confirmed it works there.

Show
George Jahad added a comment - Per RH's request, I tested this with CDT, and confirmed it works there.

People

Vote (7)
Watch (10)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.