Bug 69435 - remove WTF_CHANGES define, as it is always 1
Summary: remove WTF_CHANGES define, as it is always 1
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-05 09:37 PDT by Andy Wingo
Modified: 2015-06-29 02:33 PDT (History)
10 users (show)

See Also:


Attachments
Patch (43.21 KB, patch)
2011-10-05 09:49 PDT, Andy Wingo
no flags Details | Formatted Diff | Diff
Patch (43.44 KB, patch)
2014-02-17 10:45 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (41.67 KB, patch)
2014-02-18 04:09 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Wingo 2011-10-05 09:37:43 PDT
Hello,

I was looking through WTF today and found the WTF_CHANGES define which is always 1.  It seems that at some point some modifications were made to the malloc implementation, and the new code was under the WTF_CHANGES flag.  At some point things switched around so that old code was under #ifndef WTF_CHANGES.  That old code does not appear to be used on any platform.

This patch to be attached removes the define and related unused code.
Comment 1 Andy Wingo 2011-10-05 09:49:37 PDT
Created attachment 109812 [details]
Patch
Comment 2 WebKit Review Bot 2011-10-05 09:51:51 PDT
Attachment 109812 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/FastMalloc.cpp:3225:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Darin Adler 2011-10-05 09:54:36 PDT
Comment on attachment 109812 [details]
Patch

We created this define to help us track the original version of TCMalloc. We should not remove it just because it’s always 1. If it has outlived its usefulness we could remove it, but that decision should be made by whoever is merging in new versions of TCMalloc.
Comment 4 Andy Wingo 2011-10-05 10:13:58 PDT
(In reply to comment #3)
> (From update of attachment 109812 [details])
> We created this define to help us track the original version of TCMalloc. We should not remove it just because it’s always 1. If it has outlived its usefulness we could remove it, but that decision should be made by whoever is merging in new versions of TCMalloc.

Thank you for the review, Darin.  The define is poorly named, however, and I wonder if it really needs to make it all the way out to JavaScriptCore/config.h.  Also not all changes to tcmalloc are guarded by this flag.

Do you think that there is something worth doing here or should we close this bug?
Comment 5 Andy Wingo 2011-10-10 08:06:04 PDT
Adding Darin to cc to close.  (I am new to WebKit, so if this is the wrong thing to do, please let me know.)
Comment 6 Darin Adler 2011-10-10 14:13:52 PDT
(In reply to comment #4)
> The define is poorly named, however

I agree.

> I wonder if it really needs to make it all the way out to JavaScriptCore/config.h.

I think it’s inconvenient that it’s there. It would be nice if it wasn’t.

> Also not all changes to tcmalloc are guarded by this flag.

Also a good point.

> Do you think that there is something worth doing here or should we close this bug?

I think it’s worth finding out if we are getting any value from having our changes wrapped in #if now. If we’re not, it might be nice to reduce the noise and complexity by removing this.

I think the real question is who would know the answer to that. Who has recently dealt with comparing with the “real” TCMalloc or importing a new version of it into our source tree? I cc’d a few people who might know.
Comment 7 Andy Wingo 2011-10-17 02:07:06 PDT
Thanks for the reply, Darin.

(In reply to comment #6)
> I think it’s worth finding out if we are getting any value from having our changes wrapped in #if now. If we’re not, it might be nice to reduce the noise and complexity by removing this.
> 
> I think the real question is who would know the answer to that. Who has recently dealt with comparing with the “real” TCMalloc or importing a new version of it into our source tree? I cc’d a few people who might know.

A kind ping to those people on CC for input.

Andy
Comment 8 Darin Adler 2011-10-17 09:31:32 PDT
Comment on attachment 109812 [details]
Patch

Clearing review flag for now. Not sure we want to do this. Whoever is working on merging changes in from TCMalloc could help us decide.
Comment 9 Csaba Osztrogonác 2014-02-17 10:45:03 PST
Created attachment 224395 [details]
Patch

Updated patch for EWS. I'm going to send a mail to webkit-dev soon if we need it or not.
Comment 10 WebKit Commit Bot 2014-02-17 10:45:49 PST
Attachment 224395 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/FastMalloc.cpp:3738:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/FastMalloc.cpp:3809:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/FastMalloc.cpp:3873:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Csaba Osztrogonác 2014-02-18 04:09:59 PST
Created attachment 224492 [details]
Patch

new patch for EWS - Mac buildfix included and updated to ToT
Comment 12 WebKit Commit Bot 2014-02-18 04:18:29 PST
Attachment 224492 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/FastMalloc.cpp:3738:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/FastMalloc.cpp:3809:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/FastMalloc.cpp:3868:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Csaba Osztrogonác 2015-06-29 02:33:13 PDT
TCMalloc was removed from trunk with WTF_CHANGES define too.