Moves various exception code offsets to a central place.
Created attachment 62071 [details] Patch
Hi Darin, This is a follow up on https://bugs.webkit.org/show_bug.cgi?id=42250 The proposed patch is not yet final in the sense that I'll replicate the same idea to a few other places (if it makes sense). However, I'd appreciate your comments whether or not this is an improvement: on one hand, it turns on compile-time checking for exception code on the IDL / WebCore, and also adds a centralized place for defining the offsets, hence avoiding possible clashes. On the other hand, it adds a small burden when setting the exception code, as it now needs to offset it (rather than just use the enum value). Please, let me know your thoughts, and I'd be happy to do the same for the remaining classes. Thanks!
(In reply to comment #2) > On the other hand, it adds a small burden when setting the exception code, as it now needs to offset it (rather than just use the enum value). What I’m concerned about ost is the risk of writing the code wrong and not knowing it's wrong. It seems that now you could easily forget the offset entirely, or use the wrong offset with the wrong error code. Is there some way we can catch those mistakes at compile time?
(In reply to comment #3) > (In reply to comment #2) > > On the other hand, it adds a small burden when setting the exception code, as it now needs to offset it (rather than just use the enum value). > > What I’m concerned about ost is the risk of writing the code wrong and not knowing it's wrong. yep, that's why I didn't do on all classes at once.. ;) > > It seems that now you could easily forget the offset entirely, or use the wrong offset with the wrong error code. Is there some way we can catch those mistakes at compile time? yep, forgetting to offset is entirely possible (nit: use the wrong offset is detected at compile time though). I'm not sure if it's possible to check that the offset is being added at compile time unless we make ExceptionCode generally more opaque, but that would be far too large for probably too little benefit. one step back: another approach would be to have some attribute like "CheckEnumsWithOffset" in the IDL. we could still benefit from compile-time check of values, have the offsets centralized, safe type-based range check, but at the expense of keeping the attribute in the IDL files. let me try a quick draft on how it would look, I'll upload a new patch shortly (unless of course you think the IDL attribute is a bad idea..)
(In reply to comment #4) > I'm not sure if it's possible to check that the offset is being added at compile time unless we make ExceptionCode generally more opaque, but that would be far too large for probably too little benefit. I think making ExceptionCode be a struct holding an integer instead of actually being a raw int might be the right direction, and not such a giant project. We can phase it in, using things like a constructor taking and int and operator int at first, and then removing them.
Created attachment 62085 [details] Patch
(In reply to comment #5) > (In reply to comment #4) > > I'm not sure if it's possible to check that the offset is being added at compile time unless we make ExceptionCode generally more opaque, but that would be far too large for probably too little benefit. > > I think making ExceptionCode be a struct holding an integer instead of actually being a raw int might be the right direction, and not such a giant project. We can phase it in, using things like a constructor taking and int and operator int at first, and then removing them. cool! I like doing such refactorings, will plan to do this soon (it's probably not a giant project, but requires some planning on my side..) meanwhile, I think there's a low-hanging fruit here with the latest patch: it adds compile-time checks, without touching existing code (at the expense of having a new IDL attribute). would you mind taking a quick look and let me know if you agree with such intermediate step? thanks!
Comment on attachment 62085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=62085&action=review I'm not sure why we want to make this change. I like the idea of making ExceptionCode into a struct that has an int member. That would seem to make it much easier to use the C++ type system to help us with correctness. > WebCore/dom/ExceptionOffset.h:38 > + EVENT_EXCEPTION_OFFSET = 100, > + EVENT_EXCEPTION_MAX = 199, > + RANGE_EXCEPTION_OFFSET = 200, > + RANGE_EXCEPTION_MAX = 299, > + XML_HTTP_REQUEST_EXCEPTION_OFFSET = 500, > + XML_HTTP_REQUEST_EXCEPTION_MAX = 699, I'm not sure these are named correctly. Do we use ALL_CAPS names for similar things?
Adam’s comment got my attention. Sorry I didn’t get around to reviewing more patches this weekend! The general concept is that we should use ALL_CAPS only for: 1) Something we’re implementing from a standard, such as the DOM API, that has that capitalization. 2) Macros, especially non-function-style macros. The ALL_CAPS style there is to avoid having code actually be subject to macro expansion. Avoiding ALL_CAPS everywhere else means there is less chance of collision. These new constants don’t fit either description so should not be ALL_CAPS.