Hi *, I'd like to rename the ASSERT macro from JavaScriptCore/wtf/Assertions.h to something that doesn't clash with the Win32 ASSERT macro. There's already a hack that undefines this macro before the definition in Assertion.h. This is an easy work-around but on Windows CE (where I'm developing on) the situation is worse: ASSERT gets defined in windows.h That means ATM that I have to make sure to always include <windows.h> _before_ Assertion.h. Otherwise I come up with a mix of Windows assertions and WebKit assertions, not to speak of the warning of an already defined ASSERT... As you can see in Assertions.h, renaming ASSERT is not completely my idea: /* FIXME: Change to use something other than ASSERT to avoid this conflict with win32. */ So, what do you think about this? What would be a proper name for the macro? WKASSERT? Best Regards, Joerg
On Jun 26, 2008, at 5:16 AM, Jörg Bornemann wrote:
That means ATM that I have to make sure to always include <windows.h> _before_ Assertion.h.
Then lets add this to Assertion.h: #if PLATFORM(WINCE) // or whatever is the right if #include <windows.h> #endif I'd prefer this to a global change to the entire WebKit project. -- Darin
Darin Adler wrote:
Then lets add this to Assertion.h:
#if PLATFORM(WINCE) // or whatever is the right if #include <windows.h> #endif
I'd prefer this to a global change to the entire WebKit project.
You really want to include <windows.h> in every single webkit file? Well, this is a small change but also a very bad idea. Not because of compilation time, but because of the crappy Windows headers which define *a* *lot* of global stuff. E.g. the XSLT parser of WebKit won't build because there's a "#define ERROR <somenumber>" which breaks an enum definition. Or think of the famous MIN / MAX definitions, which drive every crossplatform developer insane. Assume, that we do this global include. Every Windows developer who wants to use WebKit must take care to avoid every possible name clash in their projects, which might be problematic, if one has to use 3rd party libraries. I'd rather live with mixed ASSERTs and compiler warnings... Best Regards, Jörg
On Jun 27, 2008, at 1:50 AM, Jörg Bornemann wrote:
Well, this is a small change but also a very bad idea. Not because of compilation time, but because of the crappy Windows headers which define *a* *lot* of global stuff. E.g. the XSLT parser of WebKit won't build because there's a "#define ERROR <somenumber>" which breaks an enum definition. Or think of the famous MIN / MAX definitions, which drive every crossplatform developer insane.
OK. Lets #undef those things. -- Darin
Darin Adler wrote:
Well, this is a small change but also a very bad idea. Not because of compilation time, but because of the crappy Windows headers which define *a* *lot* of global stuff. E.g. the XSLT parser of WebKit won't build because there's a "#define ERROR <somenumber>" which breaks an enum definition. Or think of the famous MIN / MAX definitions, which drive every crossplatform developer insane.
OK. Lets #undef those things.
This solution is easy to do, leads to the smallest source diff but is a very dirty hack, which will lead to problems on WinCE, because we will include windows.h in public headers. One survival rule of Windows developers is: only include windows.h when it is really needed. So what's your argument against the clean solution (renaming)? Regards, Jörg
On Jul 1, 2008, at 1:45 AM, Jörg Bornemann wrote:
This solution is easy to do, leads to the smallest source diff but is a very dirty hack, which will lead to problems on WinCE, because we will include windows.h in public headers.
Adding <windows.h> to Assertions.h will not cause it to be included in public headers. Assertions.h is not designed to be used in public headers; it's for internal use inside the WebKit project. And "is a very dirty hack" is not a technical argument.
So what's your argument against the clean solution (renaming)?
For one thing, I don't like the other names you suggested. We've used ASSERT for the lifetime of the WebKit project, many years. It appears in thousands of lines of code. I don't want to make a global change in that name because of a WinCE-specific issue unless there's no other solution. There are numerous examples where internal WebKit things conflict with platform headers or macros and we've been able to resolve them without renaming the WebKit things. To give one small example, we use "id" in WebKit even though that's a special reserved word on Mac OS X in Objective-C. We also manage to use min and max despite the definitions in <windef.h>. We work around these bugs in platform header design in ways that don't require us to change the bulk of the WebKit code. If your argument was that ASSERT is not a good name and you were making a case for a better name on the basis of clarity and coding style, I'd be happy to consider and debate that. Lets do the local solution in Assertions.h. Then we will have code that compiles and works on WinCE, and then we can debate the concrete merits of other solutions at our leisure. -- Darin
On Jul 1, 2008, at 10:39 AM, Paul Pedriana wrote:
On a related note, I would like to propose (possibly in a separate email) that the CRASH macro in Assertions.h that ASSERT uses be augmented to the following for improved debugging and portability across most platforms:
That sounds like something you should put in a patch in bugs.webkit.org rather than in email! -- Darin
On Jul 1, 2008, at 10:39 AM, Paul Pedriana wrote:
IMO in an ideal world ASSERT would have a unique prefix (e.g. WTF_) in order to guarantee collision avoidance as WebKit is ported to different platforms and different usage. I can see a number of other places in WebKit where this practice is already done (e.g. WEBKIT_VIDEO_SINK, KJS_FAST_CALL, etc.). This is of course similar to the reason we use C++ namespaces to help avoid code collisions.
For macros used in public headers, we definitely need a unique prefix. For macros used entirely inside WebKit, I don't think it's necessarily the best practice. But I'm open to the possibility. The C++ namespace issue is a bit different. Function names can conflict at link time in ways that macro names can't, so even functions that are not part of public interface can conflict. -- Darin
Hi Darin, Thanks for your detailed comments!
Adding <windows.h> to Assertions.h will not cause it to be included in public headers. Assertions.h is not designed to be used in public headers; it's for internal use inside the WebKit project.
I've just executed the following: find . -name '*.h' -exec grep -Hn 'Assertions.h' '{}' \; You're sure, that none of the 40+ files, the above call yields, is a public header or used inside a public header? But well, if Assertions.h is meant to be part of the private API, that particular argument of mine is void. A public header including Assertions.h, even an indirect include, should then be considered as bug?
For one thing, I don't like the other names you suggested.
Well, that was a proposal. I really don't have strong feelings about the name. I just didn't want it to be ASSERT. ;-) Regards, Jörg
On 2008-07-02, at 00:40, Jörg Bornemann wrote:
Hi Darin,
Thanks for your detailed comments!
Adding <windows.h> to Assertions.h will not cause it to be included in public headers. Assertions.h is not designed to be used in public headers; it's for internal use inside the WebKit project.
I've just executed the following: find . -name '*.h' -exec grep -Hn 'Assertions.h' '{}' \;
You're sure, that none of the 40+ files, the above call yields, is a public header or used inside a public header? But well, if Assertions.h is meant to be part of the private API, that particular argument of mine is void. A public header including Assertions.h, even an indirect include, should then be considered as bug?
On Mac OS X, Assertions.h is not installed on developer systems. This means that if any API header were to include it, applications using the API would be unable to build. I would expect that the other ports behave similarly. - Mark
On Wednesday 02 July 2008 09:40:19 Jörg Bornemann wrote:
Hi Darin,
Thanks for your detailed comments!
Adding <windows.h> to Assertions.h will not cause it to be included in public headers. Assertions.h is not designed to be used in public headers; it's for internal use inside the WebKit project.
I've just executed the following: find . -name '*.h' -exec grep -Hn 'Assertions.h' '{}' \;
You're sure, that none of the 40+ files, the above call yields, is a public header or used inside a public header? But well, if Assertions.h is meant to be part of the private API, that particular argument of mine is void. A public header including Assertions.h, even an indirect include, should then be considered as bug?
I don't think Assertions.h is used in any public header file. It's for sure not the case in the Qt port, and since it includes Platform.h I doubt it would work in any public header. Simon
participants (5)
-
Darin Adler
-
Jörg Bornemann
-
Mark Rowe
-
Paul Pedriana
-
Simon Hausmann