ClojureScript

Add warnings for invalid use of aget and aset

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test

Description

Warn on improper use of aget and aset for objects.

  1. CLJS-2148-Address-new-aget-aset-warnings.patch
    01/Jul/17 7:33 PM
    10 kB
    Mike Fikes
  2. CLJS-2148-Add-unsafe-get-and-use-goog.object.patch
    03/Jul/17 3:04 PM
    11 kB
    Mike Fikes
  3. CLJS-2148-v0.patch
    01/Jul/17 3:56 PM
    3 kB
    Mike Fikes
  4. CLJS-2148-v1.patch
    01/Jul/17 7:07 PM
    15 kB
    Mike Fikes
  5. CLJS-2148-v2.patch
    01/Jul/17 7:33 PM
    6 kB
    Mike Fikes

Activity

Hide
Mike Fikes added a comment -

Adding a v0 patch for general feedback of approach. Not meant for committing yet. Would be cleaned up, unit tests etc.

Show
Mike Fikes added a comment - Adding a v0 patch for general feedback of approach. Not meant for committing yet. Would be cleaned up, unit tests etc.
Hide
Mike Fikes added a comment -

Fleshed out implementation, added tests, and fixed the places in the std lib and tests that were abusing aget and aset per the new warnings. Attached this as -v1 patch that is worth reviewing.

Show
Mike Fikes added a comment - Fleshed out implementation, added tests, and fixed the places in the std lib and tests that were abusing aget and aset per the new warnings. Attached this as -v1 patch that is worth reviewing.
Hide
Mike Fikes added a comment -

Split -v1 patch into a -v2 patch that contains only the new warning code (along with defaulting the warnings to be off), along with a side patch that addresses all of the warnings.

Show
Mike Fikes added a comment - Split -v1 patch into a -v2 patch that contains only the new warning code (along with defaulting the warnings to be off), along with a side patch that addresses all of the warnings.
Hide
David Nolen added a comment - - edited

Removing aget in core is a perf hit for a few critical things. Let's add a new macro unsafe-get which isn't checked. There's no need for this to be private.

Show
David Nolen added a comment - - edited Removing aget in core is a perf hit for a few critical things. Let's add a new macro unsafe-get which isn't checked. There's no need for this to be private.
Hide
Mike Fikes added a comment -

The few critical places comprise native-satisfies?, hash-string, quote-string, defprotocol

Show
Mike Fikes added a comment - The few critical places comprise native-satisfies?, hash-string, quote-string, defprotocol
Hide
Mike Fikes added a comment -

The attached CLJS-2148-Add-unsafe-get-and-use-goog.object.patch is intended to replace CLJS-2148-Address-new-aget-aset-warnings.patch. It accomplishes the same overall goal, but specifically adds a new unsafe-get macro that can be used in performance critical code where goog.object/get would introduce a perf regression. It uses that new macro in the places mentioned above.

Show
Mike Fikes added a comment - The attached CLJS-2148-Add-unsafe-get-and-use-goog.object.patch is intended to replace CLJS-2148-Address-new-aget-aset-warnings.patch. It accomplishes the same overall goal, but specifically adds a new unsafe-get macro that can be used in performance critical code where goog.object/get would introduce a perf regression. It uses that new macro in the places mentioned above.
Hide
Antonin Hildebrand added a comment -

In release notes you could maybe also plug my cljs-oops library as a possible consideration for replacement

https://github.com/binaryage/cljs-oops

Show
Antonin Hildebrand added a comment - In release notes you could maybe also plug my cljs-oops library as a possible consideration for replacement https://github.com/binaryage/cljs-oops
Hide
David Nolen added a comment -

Antonin, you wish The patch w/o inference has been applied as of https://github.com/clojure/clojurescript/commit/fb6c04f4ae16877ba7fa797e9378d8a1ba74fd89

Show
David Nolen added a comment - Antonin, you wish The patch w/o inference has been applied as of https://github.com/clojure/clojurescript/commit/fb6c04f4ae16877ba7fa797e9378d8a1ba74fd89

People

Vote (1)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: