RESOLVED FIXED 130508
prepare-ChangeLog should list functions that have been removed too. 
https://bugs.webkit.org/show_bug.cgi?id=130508
Summary prepare-ChangeLog should list functions that have been removed too.
László Langó
Reported 2014-03-20 06:39:43 PDT
The prepare-ChangeLog does not list the deleted functions in the Changelog. We have to read the functions and the line ranges of them from the source before the change, then search for overlaps between them and the changed line ranges from diff.
Attachments
Patch (10.37 KB, patch)
2014-03-20 06:46 PDT, László Langó 		no flags
DetailsFormatted DiffDiff
patch for landing (10.58 KB, patch)
2014-03-24 02:13 PDT, László Langó 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
László Langó
Comment 1 2014-03-20 06:46:02 PDT
Created attachment 227287 [details] Patch
Alexey Proskuryakov
Comment 2 2014-03-20 09:09:40 PDT
On one hand, information about which functions were removed seems as relevant as information about added functions. But on the other hand, the list of functions in ChangeLog is just a template for adding per-function comments manually, and there is rarely anything to say about deleted functions ("deleted because this function is no longer needed, duh"). Overall, I'm leaning slightly against this proposed change.
László Langó
Comment 3 2014-03-21 04:02:52 PDT
(In reply to comment #2) > On one hand, information about which functions were removed seems as relevant as information about added functions. But on the other hand, the list of functions in ChangeLog is just a template for adding per-function comments manually, and there is rarely anything to say about deleted functions ("deleted because this function is no longer needed, duh"). > > Overall, I'm leaning slightly against this proposed change. I think this is a useful information. A few days ago I had to find a missing method, but I could not find out from the git blame info and neither from the ChangeLog. I wanted to know that when was it removed and why. This why I started to work on this fix. There is a TODO list in the prepare-ChangeLog, that contains this change. If we don't want this feature, then we should remove it from the list: http://trac.webkit.org/browser/trunk/Tools/Scripts/prepare-ChangeLog#L38
Darin Adler
Comment 4 2014-03-22 06:53:03 PDT
I like having the list of deleted functions. The person making the change log can always edit them out if they are not helpful.
Darin Adler
Comment 5 2014-03-22 06:56:16 PDT
Comment on attachment 227287 [details] Patch Despite Alexey’s concerns, I like doing this. If it really gets in the way we can put it behind an option. But don’t we have some sort of regression test system for prepare-ChangeLog? I seem to remember there was one. This should be covered by tests in that system if we have it.
László Langó
Comment 6 2014-03-24 01:03:07 PDT
(In reply to comment #5) > (From update of attachment 227287 [details]) > Despite Alexey’s concerns, I like doing this. If it really gets in the way we can put it behind an option. > > But don’t we have some sort of regression test system for prepare-ChangeLog? I seem to remember there was one. This should be covered by tests in that system if we have it. Not exactly for the prepare-ChangeLog, but only for the parser functions in it. Because we need a local change on source files, it's hard to write a unittest.
László Langó
Comment 7 2014-03-24 02:13:17 PDT
Created attachment 227630 [details] patch for landing
WebKit Commit Bot
Comment 8 2014-03-24 02:28:04 PDT
Comment on attachment 227630 [details] patch for landing Clearing flags on attachment: 227630 Committed r166156: <http://trac.webkit.org/changeset/166156>
WebKit Commit Bot
Comment 9 2014-03-24 02:28:10 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.