Re: [webkit-dev] renaming ASSERT macro
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
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] renaming ASSERT macro
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
smime.p7s
Description: S/MIME cryptographic signature
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] renaming ASSERT macro
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
signature.asc
Description: This is a digitally signed message part.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] renaming ASSERT macro
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 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] renaming ASSERT macro
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 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] renaming ASSERT macro
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 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] renaming ASSERT macro
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 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] renaming ASSERT macro
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 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
