Thanks for the review, Sundar. Replies inline below. > Am 20.06.2016 um 08:35 schrieb Sundararajan Athijegannathan > <sundararajan.athijegannat...@oracle.com>: > > Few questions: > > 1) So, we are allowing delete on non-configurable properties. Is this > fine from optimization PoV? i.e., we can't assume that a > non-configurable property can not be deleted.
This is indeed a problem. I wrote a test where clear is invoked on the global object between evaluation of the same script, and callsites are not invalidated properly. I don’t think it is worth to change the way global variable callsites are guarded for this change, so I’m withdrawing this RFR and closing the issue. Thanks for spotting this, Sundar. > 2) ScriptObject.clear - it appears previously, it deleted inherited > properties too. Now only immediate properties. Intended? do we have > tests to cover these cases? > No, we previously/currently also only delete properties on the object itself. Even though we use a property iterator that includes inherited properties, the deletion will only take place for properties on the object itself. I originally thought this was just an inefficiency, but now that I think of it actually is a bug as it could result in removal of non-enumerable properties. I’ll file an Jira issue for it. > 3) Global's clear method clears lexicalScope. Do we need a test for ES6 > lexical scope clear? True, but not a problem anymore since I’m not following this further. Hannes > -Sundar > > On 6/17/2016 7:24 PM, Hannes Wallnöfer wrote: >> Please review JDK-8159589: ScriptObjectMirror.clear() should remove all >> enumerable properties: >> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8159589 >> Webrev: http://cr.openjdk.java.net/~hannesw/8159589/webrev.00/ >> >> In our current implementation, ScriptObjectMirror.clear() observes JS >> semantics by not clearing properties that are non-configurable, and even >> throwing type error when invoking clear() on an object that contains such >> properties. >> >> IMO, there’s no need to follow JS semantics since we’re calling the method >> from outside of JS. With this change, clear() will remove all enumerable >> properties. This also makes the method consistent with size() and isEmpty(), >> which previously could return values > 0 and false after calling clear(). >> >> On a practical note, this is really convenient with global objects as it can >> be used to return globals to their initial state after an eval, removing all >> declared vars and functions but leaving the (non-enumerable) built-ins. >> >> Hannes >
