Confirmed users
360
edits
User:Andrew Sutherland/MailNews/GlobalDatabase/ReviewBelievedResolved: Difference between revisions
User:Andrew Sutherland/MailNews/GlobalDatabase/ReviewBelievedResolved (view source)
Revision as of 13:05, 27 January 2009
, 27 January 2009→dmose
(→dmose) |
(→dmose) |
||
| Line 909: | Line 909: | ||
Removing the contents of one list from another list is going to be O(n^2) unless you explode one of the lists to a hashset representation. Namely, list removal is O(n), we have up to n items, etc. I'm not sure there's really a more concise way to do the hashset exploding with the tools at hand, and we do have a bit of bookkeeping. | Removing the contents of one list from another list is going to be O(n^2) unless you explode one of the lists to a hashset representation. Namely, list removal is O(n), we have up to n items, etc. I'm not sure there's really a more concise way to do the hashset exploding with the tools at hand, and we do have a bit of bookkeeping. | ||
* the last half of GlodaLRUCacheCollection.delete appears to be | |||
duplicated in GlodaLRUCacheCollection.add. Factor that out into a | |||
separate method that can be shared? An _unlinkItem could be similarly | |||
factored out of a couple of methods for improved readability. Neither | |||
of these things are huge wins, though, so if you want to do them later | |||
in another bug, that's fine. | |||
I don't think the duplication level is too high, and the unlink cases are generally specialized to the case. Going to leave things as is for now. | |||
==== deletion ==== | ==== deletion ==== | ||
| Line 964: | Line 971: | ||
{{bug|463736}} | {{bug|463736}} | ||
==== query.js ==== | |||
* (Split based on action) Looks like neither WILDCARD [is not] used by any callers. Unless you know of a specific use case for them, I'm somewhat inclined to suggest axing them in the interest of simplicity. This would also allow the newQuery documentation comment in gloda.js to be made more understandable as well. On the other hand, this isn't a huge win. | |||
WILDCARD is used for full-text search; it's a sentinel object. | |||
* I think test() would be easier to grok if you split the then and else clauses of |if attribDef.singular| out into separate functions. It might be worth collapsing them into one since they're almost identical, but it's possible that the split is better for readability. | |||
Test has been refactored and is now a lot cleaner. We just promote the singular cases into the list case. | |||
* The whole structure of the APV as "sometimes [attr, param, value]" and "sometimes that stuff + other things in a contextually determined ordering" seems both hard to read and error-prone. This wants to be a hash, I suspect. | |||
The crazy APV representation has been normalized to [constraint type, attribute def, values...]. A dict might have been a good idea too; I'm coming from python-land where tuples are much much more efficient than objects/dictionaries where possible. Of course, in spider-monkey land, since dictionaries are objects, and objects can be fairly efficient with shape stuff, it's less of an issue and I should probably try and break myself of that habit. | |||
==== glautocomp.js ==== | |||
* in ACGR_addRows, why use apply? It appears to be setting |this| to what it would have been set to anyway. | |||
Previously remarked-upon; it avoids the perceived waste of concat and yet is only a single call to push. May be crazy. | |||