Compiler warning in IDBTransaction::modeToString

• Component: DOM
• Status: RESOLVED FIXED

[Philip Rogers]

http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20Heapcheck/builds/15910/steps/compile/logs/stdio

third_party/WebKit/Source/WebCore/Modules/indexeddb/IDBTransaction.cpp: In static member function 'static const WTF::AtomicString& WebCore::IDBTransaction::modeToString(WebCore::IndexedDB::TransactionMode)':
third_party/WebKit/Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:370:1: warning: control reaches end of non-void function [-Wreturn-type]

Patch forthcoming.

[Philip Rogers]

Created attachment 195683 [details]
Fix compiler warning

[Darin Adler]

Comment on attachment 195683 [details]
Fix compiler warning

This looks like a good change to me. Normally, this is our preferred style for switches on enums. We like to leave out the default case from the switch so that some compilers (gcc family ones) can warn if we fail to include one of the enum values. And we like to have an ASSERT_NOT_REACHED after the switch so that we get predictable behavior in release builds, and an assertion in debug builds, if we have a bizarre value at runtime that is not one of the enum values.

[Philip Rogers]

Comment on attachment 195683 [details]
Fix compiler warning

Thanks for the quick and educational review! I wasn't aware of the preferred enum style.

[WebKit Review Bot]

Comment on attachment 195683 [details]
Fix compiler warning

Clearing flags on attachment: 195683

Committed r147187: <http://trac.webkit.org/changeset/147187>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joshua Bell]

Interesting... I'd expect there to be a lurking compiler warning for the similarly structured IDBCursor::directionToString() function.

pdr, is it showing up anywhere?

[Philip Rogers]

(In reply to comment #6)
> Interesting... I'd expect there to be a lurking compiler warning for the similarly structured IDBCursor::directionToString() function.
> 
> pdr, is it showing up anywhere?

It looks like this bot just returns the first warning it encounters. I agree we should be seeing the same issue for directionToString() too.

The bot is now hitting the following warning:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20Heapcheck/builds/15928/steps/compile/logs/warnings%20%281%29

I've put up a patch in Chromium to fix this new warning: https://codereview.chromium.org/13378002. Obviously, this isn't a sustainable way to fix these warnings so in parallel I'll search around for a presubmit check.