242350 - LL_INIT(0, -1) is wrong

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

In bug 240257 I added some usages of LL_INIT(0, -1).

wtc said in bug 123403 comment 6 that the right way is LL_INIT(0xFFFFFFFF,
0xFFFFFFFF).

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

bug 243974's patch will fix this

Comment 2

[Darin Fisher]

-> biesi

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

fixed by bug 243974

Comment 4

[benc]

REOPEN:

The checked-in usages of this code were not fixed w/ the depends bug.

If you look at LXR, the usages checked in from bug 240257 remain.

(The first patch of bug 243974 did use LL_INIT a lot, and the 2nd patch did
remove those usages, so that bug did not add further uses...)

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

oops. well spotted, thanks benc!

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

gandalf, want to take this? a better way than comment 1 is probably to use
LL_MaxUint().

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

sure. taking

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

I hate this. I hate that reassigning does not move owner to me 

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: patch

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 167887 [details] [diff] [review]
patch

netwerk/streamconv/converters/nsDirIndexParser.cpp
-	   aIdx->SetSize(LL_INIT(0, -1)); // -1 means unknown
+	   aIdx->SetSize(LL_MaxUint());

please don't remove the comment

netwerk/streamconv/converters/nsIndexedToHTML.cpp
+    if (LL_NE((PRUint64)size, LL_MaxUint()) &&

hm, I prefer PRUint64(size) instead of (PRUint64)size..

same in xpfe/components/directory/nsDirectoryViewer.cpp

r=me with those changes; please ask darin for sr

thanks for making this patch!

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: updated patch

Comment 12

[Darin Fisher]

Comment on attachment 167893 [details] [diff] [review]
updated patch

or maybe use LL_MAXUINT instead?  (since this initializer really shouldn't
require a DSO function call on platforms where we have a native int64 type.
i know that LL_MAXUINT is just a macro for LL_MaxUint(), but I think it
should be changed.)


sr=darin with or without that change and a bug to fix LL_MAXUINT ;-)

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Use macro here

Updated patch. Biesi - could you check this in?

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

darin, gandalf: So if pruint64 is a struct (let's assume mozilla would compile
with that)... then LL_MAXUINT would be something like { PR_UINT32_MAX,
PR_UINT32_MAX }, right?

is such syntax allowed for LL_NE, member initialization in ctors, and function
arguments?

Comment 15

[Darin Fisher]

> darin, gandalf: So if pruint64 is a struct (let's assume mozilla would compile
> with that)... then LL_MAXUINT would be something like { PR_UINT32_MAX,
> PR_UINT32_MAX }, right?

no, it would not work to define LL_MAXUINT like that.  in the case where
pruint64 is not supported natively, the function calls would probably be needed.

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 168573 [details] [diff] [review]
Use macro here

<biesi> but yeah. I think LL_MaxUint() is actually better here...

marking macro patch obsolate.

Comment 17

[Darin Fisher]

> <biesi> but yeah. I think LL_MaxUint() is actually better here...
> 
> marking macro patch obsolate.

huh?  how does that logically follow?  today's macro is equivalent to
LL_MaxUint, but as i said, tomorrow's version of the macro could be optimized
for machines with a native 64-bit integer.  for machines without a native 64-bit
integer, the macro could continue to expand to the function call.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

darin: hm... it depends :) what is the purpose of this macro? maybe it's meant
for stuff like:

  PRUint64 foo = LL_MAXUINT;

for which such a {} definition would work...

Comment 19

[Darin Fisher]

(In reply to comment #18)
> darin: hm... it depends :) what is the purpose of this macro? maybe it's meant
> for stuff like:
> 
>   PRUint64 foo = LL_MAXUINT;
> 
> for which such a {} definition would work...

Sure, but you cannot tell how the macro will be used.  Therefore, it is not
possible to implement the macro using {}.  So, I would recommend that macro
expand to the function call on platforms without native 64-bit integer support.
 On other platforms, it can expand to a numeric 64-bit literal.  Make sense?

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

sure... fine with me; as long as wtc agrees with you ;)

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

I checked the LL_MAXUINT patch in.

Checking in netwerk/streamconv/converters/nsDirIndex.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/nsDirIndex.cpp,v  <--  nsDirIndex.cpp
new revision: 1.10; previous revision: 1.9
done
Checking in netwerk/streamconv/converters/nsDirIndexParser.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/nsDirIndexParser.cpp,v  <-- 
nsDirIndexParser.cpp
new revision: 1.18; previous revision: 1.17
done
Checking in netwerk/streamconv/converters/nsIndexedToHTML.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/nsIndexedToHTML.cpp,v  <-- 
nsIndexedToHTML.cpp
new revision: 1.63; previous revision: 1.62
done
Checking in xpfe/components/directory/nsDirectoryViewer.cpp;
/cvsroot/mozilla/xpfe/components/directory/nsDirectoryViewer.cpp,v  <-- 
nsDirectoryViewer.cpp
new revision: 1.117; previous revision: 1.116
done

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

after having some fun with msvc6, I ended up with:
  if (nsUint64(PRUint64(size)) != nsUint64(LL_MAXUINT) &&

instead of
  if (LL_NE(PRUint64(size), LL_MAXUINT) &&

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

filed bug 277704 for eliminating the function call in LL_ZERO etc.