Re: [webkit-dev] renaming ASSERT macro

[Paul Pedriana]

As Darin suggests, there seem to be two questions:     1 whether it's useful to have a different name for ASSERT.     2 whether it's worthwhile to make a large code change to reflect the different name. With regard to question 1, 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. But (2) it may or may not be trivial to do a replace of ASSERT project-wide to deal with a name change. Some might argue for a gradual approach, but I don't know enough about WebKit development conventions to comment on that. Even if people don't want to revise the name of ASSERT, I do think that it would be a good idea to consider prefixes for future macros. In addition to avoiding collisions, they help clarify what subsystem they are associated with. 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: #if !defined(WTF_DEBUG_BREAK)     #if defined(__GNUC__) && (defined(_X86) || defined(__i386) || defined(__i386__) || defined(i386))         #ifndef __L4ENV__             #define WTF_DEBUG_BREAK() __asm__ __volatile__ ("int3\n\tnop")         #else             #define WTF_DEBUG_BREAK() __asm__ __volatile__ ("int3; jmp 1f; 1:")         #endif     #elif defined(__GNUC__) && (defined(__powerpc__) || defined(__ppc__))         #define WTF_DEBUG_BREAK() __asm__ __volatile__ ("tw 31,1,1")     #elif defined(_MSC_VER) && (_MSC_VER >= 1300)         #define WTF_DEBUG_BREAK() __debugbreak()     #else         #define WTF_DEBUG_BREAK() (*(int *)(uintptr_t)0xffffffff = 0) #endif Paul 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

_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev