Bug 108770 – Cleanup: ExceptionCodes should be initialized to 0.

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Mike West]

The forthcoming patch is purely mechanical, the result of the following command:

    ack 'ExceptionCode [a-zA-Z0-9]+;' ./Source/ -l | xargs -L1 sed -i '' -e 's/ExceptionCode \([a-zA-Z0-9]*\);/ExceptionCode \1 = 0;/g'

There was a bit of disagreement in http://wkbug.com/108180 as to whether or not this is a good idea: ap@ asked "What is the win here? The rule has been that any code that cares about exceptionCode initializes it, and code that doesn't care doesn't initialize." I have a few responses:

1. Uninitialized variables are a bad habit. I subscribe to the broken windows theory of coding. :)
2. If/when we change our minds about ignoring these exceptions, missing initializations are a possible source of error.
3. If we're ignoring the exception, I'd prefer to do it explicitly rather than tacitly: one option is to rename the variable to 'ignoredException', another is to use 'ASSERT_NO_EXCEPTION' (or, perhaps a new 'IGNORE_EXCEPTION' macro that doesn't assert that no exception occurred, but simply ignores it). Initializations, or lack thereof, isn't a good indication of a variable's usefulness.

Finally, and least importantly to the code base, every piece we can standardize on is one less thing I have to worry about in the big chain o' sed that I'm building up for https://bugs.webkit.org/show_bug.cgi?id=98050.

[Mike West]

Created attachment 186261 [details]
Patch

[WebKit Review Bot]

Comment on attachment 186261 [details]
Patch

Attachment 186261 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16360468

[WebKit Review Bot]

Comment on attachment 186261 [details]
Patch

Attachment 186261 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16347620

[kov's GTK+ EWS bot]

Comment on attachment 186261 [details]
Patch

Attachment 186261 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16347624

[Peter Beverloo (cr-android ews)]

Comment on attachment 186261 [details]
Patch

Attachment 186261 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16352531

[Build Bot]

Comment on attachment 186261 [details]
Patch

Attachment 186261 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16360481

[Mike West]

Created attachment 186263 [details]
Patch

[Mike West]

The current patch excludes Source/WebCore/Modules/indexeddb/IDBDatabaseException.cpp from the overzealous attentions of ack|sed in order to not break IDB entirely. :)

[Mike West]

If we IGNORE_EXCEPTION away the unused 'ec' variables, then I don't think there's any disagreement on the rest. So, I'll block this on http://wkbug.com/108771, as the patch there might moot a significant portion of the disagrement here.

[Darin Adler]

Originally, long long ago, we chose not to initialize all ExceptionCode arguments for performance reasons. Are you changing this design? If so, you’ll need to announce that somewhere so people know what to do in the future.

[Alexey Proskuryakov]

> 1. Uninitialized variables are a bad habit. I subscribe to the broken windows theory of coding. :)

This is a double-edged argument. Doing work that is unnecessary (unnecessary initialization, unnecessary null checks) also has multiple negative consequences:

- It makes reading the code harder, because one can not imply nearly as much about the design from code that actively ignores the design by doing extra work.

- It makes reading the code harder, because there is more code to read.

- In certain cases, it makes the code less efficient at runtime.

I used to work on a project where member variables with default constructors had to be initialized in every constructor (e.g. every RefPtr member variable would have to be explicitly initialized as "m_foo(), m_bar()" etc.). The argument for this was essentially the same, however I don't think that you suggest going that far in this direction.

[Mike West]

Thanks for your comments.. I'm closing this bug as invalid. The same effect is better achieved with ASSERT_NO_EXCEPTION and IGNORE_EXCEPTION macros.