Summary: | Remove removeDirect | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gavin Barraclough <barraclough> | ||||
Component: | JavaScriptCore | Assignee: | Gavin Barraclough <barraclough> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cdumez, cgarcia, commit-queue, ggaren, keith_miller, mark.lam, msaboff, rniwa, saam | ||||
Priority: | P2 | ||||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Gavin Barraclough
2016-06-07 23:36:23 PDT
Created attachment 280779 [details]
Fix
Comment on attachment 280779 [details] Fix View in context: https://bugs.webkit.org/attachment.cgi?id=280779&action=review > Source/JavaScriptCore/runtime/JSObject.cpp:1506 > + // Assert to guard assumption in above comment; either there is no property in property storage, This comment seems redundant given the comment above & the assertion. Remove? > Source/JavaScriptCore/runtime/JSObject.cpp:1518 > + // Check if the property exists. > + if (isValidOffset(structure->get(vm, propertyName, attributes))) { I think we usually prefer allocating a local variable with a descriptive name like: bool propertyExists = isValidOffset(~) instead of adding comment like this at least in WebCore. > Source/JavaScriptCore/runtime/JSObject.cpp:1519 > + // Return failure if the property is non-configurable (DontDelete). This comment seems redundant. > Source/JavaScriptCore/runtime/JSObject.cpp:1523 > + // Remove the property from the structure. Ditto. > Source/JavaScriptCore/runtime/JSObject.cpp:1531 > + // If there is a slot in property storage for this property, clear it for the benefit of GC. > + if (offset != invalidOffset) I think this is another instance where a local variable with a descriptive name would help. e.g. bool shouldClearPropertySlotForGC = offset != invalidOffset; Committed revision 201834. |