303657 - cleaning up, const-ifying modules/libreg/src/reg.c

Status: RESOLVED INVALID

(Core :: XPCOM, defect)

Description

[Mikhail T.]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.4; FreeBSD; X11; amd64) KHTML/3.4.1 (like Gecko)
Build Identifier: 

The file arouse a lot of warnings, especially when compiled on amd64. I'm 
attaching the patch, which cleans and constifies it. 

Reproducible: Always

Comment 1

[Mikhail T.]

Attachment: the proposed patch

Comment 2

[Mikhail T.]

Attachment: one more potential problem fixed

The return type and the local variables of nr_GetFileLength() ought to be off_t
or, at least, long to allow it to operate on files larger than 4Gb. Not sure,
who would need a registry this large, but what do I know :-)

Comment 3

[Frank Wein [:mcsmurf]]

I don't know if you know yet :), but normally you have to request review from a
person to look at the patch. Though i'm not sure which one to pick here...

Comment 4

[Mikhail T.]

=Though i'm not sure which one to pick here... 
 
Which person, or which patch to pick? The first patch is obsoleted  by the 
second. 
 
I don't know anyone here, but the report is assigned to Doug T. Presumably, 
he'll review my patch as soon as he has time for it... But I'll welcome 
anyone's comments... 

Comment 5

[zug_treno]

(In reply to comment #4)
> I don't know anyone here, but the report is assigned to Doug T. Presumably, 
> he'll review my patch as soon as he has time for it...  

See http://www.mozilla.org/owners.html for Module Owners/Peers, it seems that
http://www.mozilla.org/owners.html#registry is the right Module for this bug.

Comment 6

[Frank Wein [:mcsmurf]]

To be honest, some people get so much bugmail that it is likely that they won't
see your patch. I'll set some review flag for you, hope it's ok.

Comment 7

[Boris Zbarsky [:bzbarsky]]

dveditz, ping?

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 191783 [details] [diff] [review]
one more potential problem fixed

For the next patch it would help reviewing if you used more lines of context and used the -p option to add hints to which routine the change is in.

This doesn't build on windows because fseeko and ftello don't exist.

>         readlen = XP_FileRead(buffer, len, fh );
>         /* PR_READ() returns an unreliable length, check EOF separately */
>+#if !defined(USE_STDIO_MODES) /* USE_STDIO_MODES means XP_FileRead = fread */
>         if (readlen < 0) {

By redefining readlen as size_t this test doesn't make sense, but PR_Read uses -1 an error when USE_NSPR_MODES is true. This section would be clearer #if defined(USE_NSPR_MODES) rather than the negation of the other, btw.

>-static REGERR nr_WriteFile(FILEHANDLE fh, REGOFF offset, int32 len, void *buffer)
>+static REGERR nr_WriteFile(FILEHANDLE fh, REGOFF offset, size_t len, const void *buffer)
>-    if ((int32)XP_FileWrite(buffer, len, fh) != len)
>+    if (XP_FileWrite(buffer, len, fh) != len)

Likewise USE_NSPR_MODES does use int32. Maybe instead of swapping problems by using size_t you should add a #define type to the various vr_stubs.h sections.

r-

Comment 9

[Boris Zbarsky [:bzbarsky]]

Asa, Mikhail says he can't update this patch because you disabled his Bugzilla access at some point... Should someone else pick this up, or can you reenable his Bugzilla access?

Comment 10

[Mikhail T.]

I think, this file should be split into clean OS-specific ones. The #ifdef spaghetti is just too much to have another look :-(

Comment 11

[Doug Turner (:dougt)]

mass reassigning to nobody.

Comment 12

[:aceman]

This is probably not needed after bug 679352.