Summary: | Consolidate notImplemented() macro definitions into header | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kevin Ollivier <kevino> | ||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | sam | ||||||||
Priority: | P2 | ||||||||||
Version: | 523.x (Safari 3) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Kevin Ollivier
2007-05-12 15:05:02 PDT
Created attachment 14521 [details]
consolidate notImplemented() macros
Comment on attachment 14521 [details]
consolidate notImplemented() macros
This seems like a good idea. A couple points you may want to address:
In NotImplemented.h
- for the non-debug version of notImplemented() you may want to use (void(0)) as we do for ASSERT instead of {}.
- the Qt and else versions should use WTF_PRETTY_FUNCTION instead of __FUNCTION__.
In WebKit/COM/ChromeClientWin.cpp
- you don't #include NotImplemented.h, you have just redefined the empty macro.
Another thing you might want to consider, though I am unsure it whether the idea will be popular, is putting the notImplemented() macros in wtf/Assertions.h which would eliminate the need to #include the NotImplemented.h header everywhere as Assertions.h should be included everywhere automatically due to config.h. Created attachment 14537 [details]
Updated patch with suggestions implemented
Created attachment 14538 [details]
Updated patch, removing ChromeClientWin change
Okay, I just uploaded a new version with changes made. (Sorry I had to upload twice, I forgot to remove the ChromeClientWin typo the first time.) Regarding where to put the macro, I did already ask on webkit-dev about putting the function into Assertions.h, and was asked to put it into a separate header, so I left that part of the patch as-is. Is there any word on this getting reviewed/landed? It's a small change that can really help to ease the building of various configurations of the ports. Maintaining the patch in the bug tracker while it waits for review is a bit pointless since it's a large patch which will rapidly diverge from TOT, so it would be best to just go ahead and get this in. Comment on attachment 14538 [details]
Updated patch, removing ChromeClientWin change
r=me assuming this still applies cleanly and you remove "WARNING: NO TEST CASES ADDED OR CHANGED" from the ChangeLog.
|