Summary: | remove WTF_CHANGES define, as it is always 1 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andy Wingo <wingo> | ||||||||
Component: | Web Template Framework | Assignee: | Csaba Osztrogonác <ossy> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | benjamin, cmarcelo, commit-queue, darin, ggaren, mrowe, msaboff, ossy, sam, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Andy Wingo
2011-10-05 09:37:43 PDT
Created attachment 109812 [details]
Patch
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 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.
(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? Adding Darin to cc to close. (I am new to WebKit, so if this is the wrong thing to do, please let me know.) (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. 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 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.
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.
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.
Created attachment 224492 [details]
Patch
new patch for EWS - Mac buildfix included and updated to ToT
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.
TCMalloc was removed from trunk with WTF_CHANGES define too. |