Bug 130508 - prepare-ChangeLog should list functions that have been removed too.
Summary: prepare-ChangeLog should list functions that have been removed too.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: László Langó
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-20 06:39 PDT by László Langó
Modified: 2014-10-20 23:15 PDT (History)
5 users (show)

See Also:


Attachments
Patch (10.37 KB, patch)
2014-03-20 06:46 PDT, László Langó
no flags Details | Formatted Diff | Diff
patch for landing (10.58 KB, patch)
2014-03-24 02:13 PDT, László Langó
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description László Langó 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.
Comment 1 László Langó 2014-03-20 06:46:02 PDT
Created attachment 227287 [details]
Patch
Comment 2 Alexey Proskuryakov 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.
Comment 3 László Langó 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
Comment 4 Darin Adler 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.
Comment 5 Darin Adler 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.
Comment 6 László Langó 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.
Comment 7 László Langó 2014-03-24 02:13:17 PDT
Created attachment 227630 [details]
patch for landing
Comment 8 WebKit Commit Bot 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>
Comment 9 WebKit Commit Bot 2014-03-24 02:28:10 PDT
All reviewed patches have been landed.  Closing bug.