Bug 191193 – Metadata should not be copyable

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Tadeu Zagallo]

...

[Tadeu Zagallo]

Created attachment 353702 [details]
Patch

[Keith Miller]

r=me.

[WebKit Commit Bot]

Comment on attachment 353702 [details]
Patch

Clearing flags on attachment: 353702

Committed r237734: <https://trac.webkit.org/changeset/237734>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45762874>

[David Kilzer (:ddkilzer)]

<rdar://problem/45682358>

[Saam Barati]

Comment on attachment 353702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353702&action=review

> Source/JavaScriptCore/ChangeLog:8
> +        We should only ever hold references to the entry in the metadata table.

Why? What about a test for this?

[Tadeu Zagallo]

Comment on attachment 353702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353702&action=review

>> Source/JavaScriptCore/ChangeLog:8
>> +        We should only ever hold references to the entry in the metadata table.
> 
> Why? What about a test for this?

Because the metadata is mutable, so it's nice to avoid cases like in CodeBlock below, where I was accidentally copying and then mutating a local copy. You're right, I should have added a test. I'll add it now.

[Mark Lam]

Comment on attachment 353702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353702&action=review

>>> Source/JavaScriptCore/ChangeLog:8
>>> +        We should only ever hold references to the entry in the metadata table.
>> 
>> Why? What about a test for this?
> 
> Because the metadata is mutable, so it's nice to avoid cases like in CodeBlock below, where I was accidentally copying and then mutating a local copy. You're right, I should have added a test. I'll add it now.

I think this is Saam’s way of asking you to add the “why” to the ChangeLog to document this reasoning for posterity.  Can you add your explanation in the ChangeLog here, particularly the part about the sort of bugs this prevents?  Thanks.

[Tadeu Zagallo]

Comment on attachment 353702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353702&action=review

>>>> Source/JavaScriptCore/ChangeLog:8
>>>> +        We should only ever hold references to the entry in the metadata table.
>>> 
>>> Why? What about a test for this?
>> 
>> Because the metadata is mutable, so it's nice to avoid cases like in CodeBlock below, where I was accidentally copying and then mutating a local copy. You're right, I should have added a test. I'll add it now.
> 
> I think this is Saam’s way of asking you to add the “why” to the ChangeLog to document this reasoning for posterity.  Can you add your explanation in the ChangeLog here, particularly the part about the sort of bugs this prevents?  Thanks.

Yeah, I thought that might be the case. Is it ok to edit a previous ChangeLog entry? Should I add a new ChangeLog entry to say that I'm editing a previous ChangeLog entry?

[Saam Barati]

(In reply to Tadeu Zagallo from comment #10)
> Comment on attachment 353702 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353702&action=review
> 
> >>>> Source/JavaScriptCore/ChangeLog:8
> >>>> +        We should only ever hold references to the entry in the metadata table.
> >>> 
> >>> Why? What about a test for this?
> >> 
> >> Because the metadata is mutable, so it's nice to avoid cases like in CodeBlock below, where I was accidentally copying and then mutating a local copy. You're right, I should have added a test. I'll add it now.
> > 
> > I think this is Saam’s way of asking you to add the “why” to the ChangeLog to document this reasoning for posterity.  Can you add your explanation in the ChangeLog here, particularly the part about the sort of bugs this prevents?  Thanks.
> 
> Yeah, I thought that might be the case. Is it ok to edit a previous
> ChangeLog entry? Should I add a new ChangeLog entry to say that I'm editing
> a previous ChangeLog entry?

Sorry for the ambiguity. I did mean the “why” as a way of asking for this to be documented in the change log. Personally, I’ve never gone back and edited previous change log entries. I’m not sure what the official WebKit stance on that is. I think this bugzilla will act as a point of reference if anyone needs to dig into this more in the future. You could also document it in the changelog for the added test, but that won’t be found as easily as the original commit.