[05:30:25] TimStarling: I have some ideas on how to optimize getPrefixedText()/parseTitleString(), but I'm not really sure what to do about all the Assert:: stuff besides re-implementing the makeTitle/makeTitleSafe distinction for TitleValue, where only the latter will do all the sanity checks [05:33:04] the Assert::parameterType() calls could probably be replaced by scalar type hints [05:33:21] maybe the effect is not exactly the same but it's not really important [05:33:55] the regex thing should definitely be removed [05:34:15] the last one, $dbkey !== '', could be inlined [05:34:33] if ( $dbkey==='' ) { throw ... } [05:34:35] we can't use scalar type hints until we drop HHVM support [05:38:08] well, all of them could be profitably inlined, it would be verbose but faster [05:39:11] ok. so then what's the point of having a library for assertions if it's too slow to use? [05:39:25] I don't know [05:39:38] maybe there are some cases when it is not too slow [05:39:52] but it should never have been used for this [05:40:09] but you know Daniel, it is hard to convince him of anything [05:40:12] https://codesearch.wmflabs.org/core/?q=Assert%3A%3A&i=nope&files=&repos= [05:41:21] it's obviously faster to use is_int() than Assert::parameterType(), look at the implementation of the latter [05:41:57] not that anyone cares if an integer is passed as a float, coercing is the right thing to do in that case [05:42:23] * legoktm files some follow-up tasks [05:47:43] the Assert library came out of a wikitech-l discussion in which I was saying the PHP function assert() is terrible and we should never use it, and Daniel saying that he desperately needed something like it for Wikidata [05:47:59] so I said write your own library if you need it so much, and he did [05:50:35] https://lists.gt.net/wiki/wikitech/378815#378815 [05:51:31] the eval() part is deprecated in 7.2, and supposedly will be removed in 8.0 [05:54:13] Assert::parameter() looks fine, I think ::parameterType() is the main problem [05:57:24] https://phabricator.wikimedia.org/T201802 [05:58:26] well, we can have Assert::parameterIsInt() [11:12:12] tgr: Revisionrenderer is read for review: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/451025 [13:09:05] DanielK_WMDE_: ack, will look [18:04:11] legoktm: TimStarling: Hm.. yeah, if we're okay with calling is_int directly, I'd almost think it could just be inlined always. One benfit of if(is_int){throw} over assert(is_int) is that the former would allow throwing a more specific exception and generally more explicit. [18:04:29] The only one left at that point would be array entries type matching, for that a utility might still be useful. [18:04:41] btw, would https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/447017/ is ready for re-review when you have a moment :)