mozilla :: #maildev

15 Mar 2017
00:08acemanbustage again
00:09acemanNo local packages or working download links found for blessings>=1.3 error: Could not find suitable distribution for Requirement.parse('blessings>=1.3')
00:46jorgkaceman: Sorry. Wrong comment.
00:47ewongwsmwk: gonna push that patch
00:49acemanjorgk: ok
00:53jorgkaceman: maybe just initialise entry there.
00:54acemanI think we do not usually do that for pointers
00:55acemanthe bug was that entry wasn't touched if .Get() failed
00:55acemanbut it returns false in case of failure so I test that now
00:57jorgkwe don't initalise nsCOMPtr since the constructor already nulls the raw pointer in the object.
00:57jorgkAny other variable should be initialised.
00:57acemanisn't it done by the compiler?
01:00acemanso can we do both?
01:01jorgkthat depends in the class.
01:01acemanI mean, what if Get() decides to return random garbage in entry when it is returning false?
01:01jorgkSimple things are just allocated.
01:02jorgkWell, typically those functions shouldn't touch the output unless they work.
01:02ewongwsmwk: pushed.
01:02jorgkIn you're solution I need to understand Get.
01:03jorgkIn my solution I just need common sense.
01:04jorgkAnd I don't like that the function or whatever that Get is returns a value and polulates entry as a *side effect*.
01:04jorgkTypically such code reads:
01:04jorgktype bla = nullprt;
01:04acemanbut common sense may not be obeyed by code ;)
01:05acemanbut the published api (only look at the return value) should be
01:05jorgkif (NS_SUCCEEDED(func(&bla)) && bla) {
01:07jorgkWhere is that Get defined? Some hash table lookup, right?
01:07jorgkAnd the entries are never null?
01:08acemanhttps://dxr.mozilla.org/comm-central/rev/76ceeecd75c4116b24134f1ed8d7950023c07bf5/mozilla/xpcom/ds/nsBaseHashtable.h#84
01:09acemanwe can agree on that pattern
01:11acemanalso notice in other functions in that file they check return of .Get(), not entry
01:12jorgkhuh?
01:13jorgkGet returns the data, not the entry.
01:13acemanno
01:13acemanthere are 2 versions of GET
01:13acemanI don't know what the entries are, but they seem to be objects, they call its methods, so shouldn't be null
01:13jorgkWell, at line #84 it returns the data.
01:13acemanwe use the Get at line 84 I assume
01:14acemanwhy?
01:14acemanthere is return true or false
01:14jorgkthat that can return null
01:14acemanthe one at 108 returns the data
01:14jorgkit returns ent->mData on the aData parameter.
01:15jorgkAnd that can be null.
01:15jorgkAnd if you don't test that, you crash.
01:15jorgkSo the code you sent isn't right.
01:15jorgksuccess in finding the hash entry doesn't garantee that the value of that entry isn't null.
01:16jorgk... and you crash.
01:16jorgkIf you want you can do:
01:16jorgkif (mServers.Get(nsDependentString(aKey), &entry) && entry) {
01:17acemanyes, I said I would agree with that ;)
01:17jorgkNow, maybe there is logic that those values can't be nullptr.
01:17jorgkBut I don't know that reading the code.
01:17acemanwhen you see all the .Put() I don't think they ever store nullptr
01:18acemanbut no problem
01:18acemanlet's be robust
01:19jorgkrobust is the line I gave you.
01:19acemanyes
01:19jorgkDid it return an entry and is that entry not null.
01:19acemanshould I do it at each call of .get()?
01:19jorgkHow many are there?
01:20aceman~10
01:21jorgkBut they don't all access the returned entry.
01:21jorgkFrankly, I'd do as little as possible.
01:21jorgkMy solution would be: intialise entry = nullprt and be done with it.
01:22acemanok
01:22jorgkleave everything else, since the Get call is well-behaved. Not entry found, no output modified.
01:25jorgkI gotta go to bed. 2:30 is too late for me.
01:25acemanyes, good night
01:28jorgkhmm, you did change the if?
01:28jorgkDidn't we agree to just inititialise?
01:28jorgkI don't need to initialise if I do the extra test.
01:29aceman(02:16:50) jorgk: If you want you can do:
01:29aceman(02:16:57) jorgk: if (mServers.Get(nsDependentString(aKey), &entry) && entry) {
01:29jorgksure, but later: My solution would be: intialise entry = nullprt and be done with it. - leave everything else, since the Get call is well-behaved. No entry found, no output modified.
01:30jorgkthat's all the compiler cares about and it's the minimal change here.
01:31acemanI don't want to rely that Get stays well-behaved
01:31acemanall the other places in that file also do not reply on it
01:33jorgklike where?
01:34acemaneverywhere
01:34acemannsLDAPService.cpp
01:34acemanthey only check return value of Get()
01:35acemanI'm fine with checking entry in addition to that
01:35acemanbut not instead
01:35acemanwhy should this one place be different?
01:35jorgkOK.
01:35jorgkI looked at the others.
01:35jorgkSo let's do: if (mServers.Get(nsDependentString(aKey), &entry) && entry) {
01:36jorgkbut with no intialisation.
01:36jorgkThat was the alternative, not additionally.
01:36jorgkNone of the others intialise, as far as I can see.
01:36acemanok
01:36jorgkanother patch coming?
01:37acemanyes
01:37jorgkAnd can you land that?
01:38jorgkThere have been 88 M-C changesets since the last C-C build
01:38acemanthere a build bustage but only on linux debug, maybe it is intermittent
01:38jorgkactually: 93
01:38acemanyes I can land it
01:41jorgkactually, only 25, 93 since 0:00 but Prtrick landed something at 0:44.
01:45jorgkaceman: Maybe better keep it for later.
01:45acemanok
01:45acemangood night then
01:46acemanpaenglab has another in stock, just not marked checkin-needed
01:46jorgkOK. Good night.
05:57marcoagpintohello
12:36marcoagpintohey hey hey
12:36marcoagpintothe demon!
21:11jorgkrkent: Hi Kent, I thought I'd say "hello" when I don't want anything for a change.
22:10acemanhi
22:25jorgkaceman: Hi, I didn't go bing. I'll talk to you in a little while, a little busy now.
22:27acemanthat's intentional :)
22:32jorgknew contributor with excellent patch, bug 1231592, will land now.
22:32firebothttps://bugzil.la/1231592 ASSIGNED, gds@chartertn.net With Charter ISP's IMAP server, deleted or marked as spam emails won't restore to inbox and are lost
22:32jorgkacemanL what about the wrong date?
22:32jorgkaceman: what about the wrong date?
22:33acemanposted comment
22:53jorgkaceman: Posted reply.
23:03acemanundefined seems to be according to spec too, but [] is shorter :)
23:03acemanI'll see what Magnus likes
23:11jorgkwell, you'll have to please the reviewer. And since everyone else is using 'undefined', we should go with the flow.
23:13acemanwho is everyone?
23:13jorgkthe other bugs. The calendar one, and Aryx with his two bugs.
23:14acemanI think Aryx does what we do so he can switch
23:15acemanthe chat actually sets a locale, but it they drop it, that can also do what we all agree to
23:16acemanif they drop it, due to the new defaults in m-c
23:20jorgkthat leaves the calendar bug. Would you like to lobby something else there?
23:21acemanI don't care much, I just say that at this point we can still choose anything, those patches aren't done yet
23:27jorgkaceman: OK, Are you going to attach an updated patch so I can land something tomorrow morning?
23:28acemanhardly, as we wait for Magnus now
23:28acemanbut this bug is in no hurry now
23:37jorgkOK, good night.
23:38acemanbye
16 Mar 2017
No messages
   
Last message: 42 days and 11 hours ago