...
Created attachment 353702 [details] Patch
r=me.
Comment on attachment 353702 [details] Patch Clearing flags on attachment: 353702 Committed r237734: <https://trac.webkit.org/changeset/237734>
All reviewed patches have been landed. Closing bug.
<rdar://problem/45762874>
<rdar://problem/45682358>
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?
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.
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.
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?
(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.