113547 2013-03-28 17:11:21 -0700 Compiler warning in IDBTransaction::modeToString 2013-03-29 20:03:50 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 pdr pdr alecflett dgrogan jsbell webkit.review.bot oldest_to_newest 865928 0 pdr 2013-03-28 17:11:21 -0700 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. 865932 1 195683 pdr 2013-03-28 17:19:46 -0700 Created attachment 195683 Fix compiler warning 865934 2 195683 darin 2013-03-28 17:22:56 -0700 Comment on attachment 195683 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. 865937 3 195683 pdr 2013-03-28 17:32:33 -0700 Comment on attachment 195683 Fix compiler warning Thanks for the quick and educational review! I wasn't aware of the preferred enum style. 865942 4 195683 webkit.review.bot 2013-03-28 17:48:34 -0700 Comment on attachment 195683 Fix compiler warning Clearing flags on attachment: 195683 Committed r147187: <http://trac.webkit.org/changeset/147187> 865943 5 webkit.review.bot 2013-03-28 17:48:37 -0700 All reviewed patches have been landed. Closing bug. 866220 6 jsbell 2013-03-29 10:13:54 -0700 Interesting... I'd expect there to be a lurking compiler warning for the similarly structured IDBCursor::directionToString() function. pdr, is it showing up anywhere? 866540 7 pdr 2013-03-29 20:03:50 -0700 (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. 195683 2013-03-28 17:19:46 -0700 2013-03-28 17:48:34 -0700 Fix compiler warning 113547.1.patch text/plain 1545 pdr SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE0NzE4NSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBACisyMDEzLTAzLTI4ICBQaGlsaXAg Um9nZXJzICA8cGRyQGdvb2dsZS5jb20+CisKKyAgICAgICAgRml4IGNvbXBpbGVyIHdhcm5pbmcg aW4gSURCVHJhbnNhY3Rpb246Om1vZGVUb1N0cmluZworICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTEzNTQ3CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKKyAgICAgICAgTm90IGFsbCBjb21waWxlcnMgYXJlIHNtYXJ0IGVub3Vn aCB0byBoYW5kbGUgdGhpcyBzd2l0Y2guIFRoaXMgcGF0Y2ggcmVmYWN0b3JzCisgICAgICAgIG1v ZGVUb1N0cmluZyB0byBub3QgcmV0dXJuIGZyb20gdGhlIGRlZmF1bHQgY2FzZS4KKworICAgICAg ICBObyBuZXcgdGVzdHMgYXMgdGhpcyBpcyBqdXN0IGEgY29tcGlsZXIgZml4LgorCisgICAgICAg ICogTW9kdWxlcy9pbmRleGVkZGIvSURCVHJhbnNhY3Rpb24uY3BwOgorICAgICAgICAoV2ViQ29y ZTo6SURCVHJhbnNhY3Rpb246Om1vZGVUb1N0cmluZyk6CisKIDIwMTMtMDMtMjggIFJhZmFlbCBX ZWluc3RlaW4gIDxyYWZhZWx3QGNocm9taXVtLm9yZz4KIAogICAgICAgICBbSFRNTFRlbXBsYXRl RWxlbWVudF0gPHRlbXBsYXRlPiBzaG91bGQgYmUgYWJsZSB0byBiZSBhIGZvc3RlciBwYXJlbnQK SW5kZXg6IFNvdXJjZS9XZWJDb3JlL01vZHVsZXMvaW5kZXhlZGRiL0lEQlRyYW5zYWN0aW9uLmNw cAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL2luZGV4ZWRkYi9JREJUcmFu c2FjdGlvbi5jcHAJKHJldmlzaW9uIDE0NzEyNikKKysrIFNvdXJjZS9XZWJDb3JlL01vZHVsZXMv aW5kZXhlZGRiL0lEQlRyYW5zYWN0aW9uLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMzYyLDExICsz NjIsMTAgQEAgY29uc3QgQXRvbWljU3RyaW5nJiBJREJUcmFuc2FjdGlvbjo6bW9kZQogICAgIGNh c2UgSW5kZXhlZERCOjpUcmFuc2FjdGlvblZlcnNpb25DaGFuZ2U6CiAgICAgICAgIHJldHVybiBJ REJUcmFuc2FjdGlvbjo6bW9kZVZlcnNpb25DaGFuZ2UoKTsKICAgICAgICAgYnJlYWs7Ci0KLSAg ICBkZWZhdWx0OgotICAgICAgICBBU1NFUlRfTk9UX1JFQUNIRUQoKTsKLSAgICAgICAgcmV0dXJu IElEQlRyYW5zYWN0aW9uOjptb2RlUmVhZE9ubHkoKTsKICAgICB9CisKKyAgICBBU1NFUlRfTk9U X1JFQUNIRUQoKTsKKyAgICByZXR1cm4gSURCVHJhbnNhY3Rpb246Om1vZGVSZWFkT25seSgpOwog fQogCiBjb25zdCBBdG9taWNTdHJpbmcmIElEQlRyYW5zYWN0aW9uOjppbnRlcmZhY2VOYW1lKCkg Y29uc3QK