Brendan Eich [:brendan] Reporter
	Description • 20 years ago

See bug 265921 comment 5.  Attachment of bad PRBool assignment statements next,
followed by XPConnect patch to assert on non-1 truth values flowing from native
code into JS.

/be

	Brendan Eich [:brendan] Reporter
	Comment 1 • 20 years ago

Comments welcome.

/be

	Brendan Eich [:brendan] Reporter
	Comment 2 • 20 years ago

Haven't had time to try this yet, it may be painful at first ;-).

/be

	David Bradley
	Comment 3 • 20 years ago

Probably what would be easier is to declare PRBool as bool, compile with VC++ 7
using warning level 4, or gcc if it has a similar warning, and look for the
specific error, for instance C4305 for VC++ 7. That will catch pretty much
anything that doesn't assign PR_TRUE or PR_FALSE to a PRBool

	David Bradley
	Comment 4 • 20 years ago

I attempted this tonight but I'm having to deal with various assumptions that
PRBool is PRIntn for instance int nsHashtable's EnumFunc typedef. I'll post a
bug on that tomorrow after I double check to see what it should be. Not sure
what other fun I'll find along the way, but hopefully I'll get the compiler to
generate output that I can grept the warning list.

	David Bradley
	Comment 5 • 20 years ago

Three days later and I'm still wading through compiler errors. Hopefully soon
I'll have a patch and a list of problems.

	David Bradley
	Comment 6 • 20 years ago

This is the list of warnings (Despite the file name in the zip). I'm going to
work on the error patch, I want to try another thing before I submit it.

We need to move on this quickly, as the line numbers and patch are going to get
stale quickly.

There are over 1600 warnings in the list.

	David Bradley
	Comment 7 • 20 years ago

This is the patch I ended up with. It does two things, it typedef's PRBool to
bool for C++ code, for C it uses an enumeration. I'm not suggesting we check in
the change to the PRBool typedef. I wanted to post it in case someone wanted to
try this as well.

Also just a note about the previous warning list I posted. This is a distilled
list. Not a raw compiler output. I consolidated all the duplicate warnings for
the same file/line.

	Christopher Aillon (sabbatical, not receiving bugmail)
	Comment 8 • 20 years ago

Comment on attachment 163933 [details] [diff] [review]
Fixes errors, and contains PRBool typeded to bool

>Index: nsprpub/pr/include/prtypes.h
>===================================================================
>RCS file: /cvsroot/mozilla/nsprpub/pr/include/prtypes.h,v
>retrieving revision 3.20.4.8
>diff -u -p -r3.20.4.8 prtypes.h
>--- nsprpub/pr/include/prtypes.h	28 Apr 2004 00:33:43 -0000	3.20.4.8
>+++ nsprpub/pr/include/prtypes.h	30 Oct 2004 04:36:33 -0000
>@@ -444,9 +444,17 @@ typedef unsigned long PRUptrdiff;
> **      'if (bool)', 'while (!bool)', '(bool) ? x : y' etc., to test booleans
> **      juast as you would C int-valued conditions. 
> ************************************************************************/
>-typedef PRIntn PRBool;
>-#define PR_TRUE 1
>-#define PR_FALSE 0
>+#ifdef __cplusplus
>+typedef bool PRBool;
>+bool const PR_TRUE = true;
>+bool const PR_FALSE = true;


Oops?  :-)


>+#else
>+typedef enum
>+{
>+    PR_FALSE = 0,
>+    PR_TRUE = 1
>+} PRBool;
>+#endif

	David Bradley
	Comment 9 • 20 years ago

Yes, that was unintential, must of occured after a few tricks I tried to get
things to work in both C and C++.

Just a note, the build doesn't run. I expected as much given the dependance of
XPTCall on types and sizes.

	Brendan Eich [:brendan] Reporter
	Comment 10 • 18 years ago

Taras, care to take this one? Low priority, might be an easy one to fix with an oink tool.

/be

	(dormant account) Assignee
	Comment 11 • 18 years ago

S(In reply to comment #10)
> Taras, care to take this one? Low priority, might be an easy one to fix with an
> oink tool.

Sure. This was impossible to fix before integrating CPP into oink. I should be able to tackle this in the near future. Should I assign the bug to myself?

	(dormant account) Assignee
	Comment 12 • 18 years ago

I think I am mostly done the detection part of my tool. Questions:
1) How should I handle conversion from other bool types (such as PRPackedBool)
I heard that PRPackedBool can end up with negative values, not sure of the details. I'd also need to make a list of bool types that can be assigned directly to PRBool. Alternatively 2) would handle it.

2) Looks like there is a lot of these, should I bother with producing patches automatically? I'm guessing the answer is yes. This would be as simple as wrapping the expression in !!(expr). Shouldn't take more than a few days to implement.

	(dormant account) Assignee
	Comment 13 • 18 years ago

Err. I meant this is a sample list. This is a result of running prcheck on a single file I use for testing.

	Brendan Eich [:brendan] Reporter
	Comment 14 • 18 years ago

Quick reply to comment 12:

1a. PRPackedBool is uint8, so no worries about negatives. You may have been hearing concern about folks using enum-typed single-bit fields, extracting them as PRBool and getting -1 or 0xffffffff on some platforms.

1b. Packed bool can be assigned to bool (JS or PR) type, no problem. Other way wants a cast to silence warnings, and this checker to make sure we aren't trying to store 0x100 (a flag bit masked off from a flags bit-set) into a PRPackedBool, which of course would be a bad bug ("true" turning into false).

2. Automated patches seem to me the only way to fix this big/moving-target code patching problem. The only question is one of style: !!E vs. E != 0. I have so far preferred the latter, and it's less odd-looking to most C and C++ hackers.

Comments from cc: list welcome.

/be

	(dormant account) Assignee
	Comment 15 • 18 years ago

Good news: I think I'm done with my tool. Attached is the patch output to replace the previous error reports.
It checks return statements in prbool methods, prbool function arguments, variable assignments. It checks JSBool, PRBool and PRPackedBool. Eventually I'll split these checks up to treat some of the types like booleans and some like boolean-compatible ones.
Bad news: MCPP, the C++ processor I'm using can't grok Mozilla yet. It worked fine for the files I was testing but now seems to screw up on many others. I'm looking into it, to see if it can be fixed or if I should just hack up gcc to do what I want. This means that I can't process many files. Tomorrow I'll submit as large of patch as possible given that I can only parse the subset of Mozilla.

Does this patch look good? I'm going to put up the tool in a public repository later this week, would be great if someone could run it through some corner cases.

	(dormant account) Assignee
	Comment 16 • 17 years ago

Does anyone want to start committing incremental fixes for this? 
Here is what I can produce currently. There places where code can not be rewritten automatically. This is mostly the case in functions that use PRBool return type and use NS_ENSURE* or similar macros.
There are a few bugs left in my tool mostly to do with pointers. Those should be fixed soon.

	(dormant account) Assignee
	Comment 17 • 17 years ago

I'm not sure where to take it from here. There are (2-3) places in the code that expect PRBool contain stuff other than 0/1. Also the patch is gigantic, so it would have to be committed in pieces. 
Indentation is a little messed up, but I fix it up automatically in most cases.

Comments?

	Blake Kaplan (:mrbkap) (inactive)
	Comment 18 • 17 years ago

I glanced briefly at the parser/htmlparser/src/CNavDTD.cpp changes and unfortunately, they're incorrect. This is because the code in question is treating PRBool as a tri-state (where -1 is unknown) variable. I'll file a bug and fix it manually, just thought I'd point it out.

	(dormant account) Assignee
	Comment 19 • 17 years ago

Attached is a list of locations that may return non 0/1 values through macros.

Just to clarify. The patch produced isn't supposed to be a correct fix, but more of a best-guess fix. Main purpose to highlight prbool misuse.

	Eli Friedman
	Comment 20 • 17 years ago

What exactly is wrong in nsContentList.cpp?

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 17 years ago

Fun.  Cache service defines boolean-valued constants to pass in...

-          PRInt32 semiOffset = style.Find("ch", widthOffset+6);
+          PRInt32 semiOffset = style.Find("ch", 0 != (widthOffset+6));
                             : style.Length() - widthOffset);

is wrong.  That second arg is not a PRBool...

For the rest, if a function returns PRBool, it should be ok to assign it to a PRBool. 

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 17 years ago

Although that string overloading is pretty sucky:

285   NS_HIDDEN_(PRInt32) Find(const char *aStr, PRBool aIgnoreCase = PR_FALSE) const
286   { return Find(aStr, 0, aIgnoreCase); }
287 
288   NS_HIDDEN_(PRInt32) Find(const char *aStr, PRUint32 aOffset, PRBool aIgnoreCase = PR_FALSE) const;

I wonder whether it actually works right...  I guess with the unsigned vs signed thing (PRBool is signed) it does as long as your offset is unsigned.

	(dormant account) Assignee
	Comment 23 • 17 years ago

Eli: 
nothing is wrong with nsContentList.cpp, it's a bug in my tool, thanks.

Boris:
I checked the code and it's actually calling the PRBool overload so this is a bug in Mozilla. In this case the offset is signed so it calls the wrong one.

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 17 years ago

> I checked the code and it's actually calling the PRBool overload 

Fun.  Should be [possible to write a unit test for this, I would think; we should make sure we do that when we make that change.

It sounds like the simplest way to proceed here is to use this as a tracking bug and file separate bugs (per-module perhaps?) for the changes so they can get reviewed...

	Eli Friedman
	Comment 25 • 17 years ago

I think there's a bug in the patch-making tool in nsInlineFrame.h.  The patch doesn't correspond to the actual text of the file.

	(dormant account) Assignee
	Comment 26 • 17 years ago

In reply to comment #25)
> I think there's a bug in the patch-making tool in nsInlineFrame.h.  The patch
> doesn't correspond to the actual text of the file.
> 

It corresponds to the file here. To make the patch I applied the generated patch to my checkout and then used cvs diff to get a nicer looking patch with window support. Are you sure that your file is recent and on the same branch? I'm using firefox trunk.

	Eli Friedman
	Comment 27 • 17 years ago

   PRBool IsLeftMost() const {
     // If the frame's bidi visual state is set, return is-leftmost state
     // else return true if it's the first continuation.
-    return (GetStateBits() & NS_INLINE_FRAME_BIDI_VISUAL_STATE_IS_SET)
+    return 0 != ((GetStateBits() & NS_INLINE_FRAME_BIDI_VISUAL_STATE_IS_SET)
-             : (!GetPrevInFlow());
+             : (!GetPrevInFlow()));
   }

vs.

  PRBool IsLeftMost() const {
    // If the frame's bidi visual state is set, return is-leftmost state
    // else return true if it's the first continuation.
    return (GetStateBits() & NS_INLINE_FRAME_BIDI_VISUAL_STATE_IS_SET)
             ? (GetStateBits() & NS_INLINE_FRAME_BIDI_VISUAL_IS_LEFT_MOST)
             : (!GetPrevInFlow());
  }

	(dormant account) Assignee
	Comment 28 • 17 years ago

Oops, the patch I was looking at on my hd and the one I uploaded were different. The one I uploaded before was a result of silly bad grep -v \?

Sorry about that.

	(dormant account) Assignee
	Comment 29 • 17 years ago

This is a result of a few days of work to get better typedef support in prcheck. Elsa does not provide typedef info in the typesystem, so I had to overlay this on manually. As a result all of the callbacks that were wrongly flagged before are gone now. 
Since chose not to reimplement the C++ typechecker, prcheck will still balk on complicated casts or more complicated typedef indirections. There should be very few of these problem cases.
Would be nice if someone started committing fixes.

	Kuno Meyer
	Comment 30 • 17 years ago

I just looked on the latest attachment. Wouldn't it make more sense to introduce a PR_NONZERO() macro? Code in the form of ||someBool = 0 != someInt|| looks very distracting to me in terms of operator precedence.

Or a PR_CHECKFLAG(someInt, SOMEFLAG) for the (frequent) cases of ||somebool = 0 != (val & SOMEFLAG)||

Just my two cents.

	Brendan Eich [:brendan] Reporter
	Comment 31 • 17 years ago

I agree it's confusing to see boolvar = 0 != (val & FLAG), but yet another macro is worse. Suggest boolvar = !!(val & FLAG); or boolvar = (val & FLAG) != 0 (put the zero on the right. If precedence is a concern (it isn't for me -- = is loser than all but the comma operator), parenthesize the right hand side.

/be

	Kuno Meyer
	Comment 32 • 17 years ago

It sounds reasonable not to introduce any more macros. On the other hand, you could rectify this with the argument that for a macro-based type the operators could be macro-based as well.

Putting || != 0 || to the end seems even worse to me, because it tends to get overlooked and forgotten while editing/reorganising.

For me, the most readable of your suggestions is the parenthesizing in the case of assignments to variables. || boolVal = (0 != intVal) ||. For function arguments or return values, this is not needed.

	(dormant account) Assignee
	Comment 33 • 17 years ago

another trick you can do is do static_cast<bool>(). Putting parens around everything tends to get silly in some points, so some special casing is required. 

You can try customizing the tool to make things look nicer. I already have a few special cases. For example I usually rewrite expressions in the form of 0 != (exp) unless exp is a variable, a function call, etc - then I use 0 != exp.

I also tried to make use of prbool casts. Whenever something is cast to a prbool, I rewrite the expression within the cast, instead of outside of a cast.

I don't see a general solution that will be aesthetically appealing in all cases. I also don't think it matters a whole lot, since the patch is only a suggested correction. It's often better to rewrite code with funny precedence and simplify it by hand. 

Most of the patch is fixes for misuses of & operator which are can be applied as is, the rest of too little to bother writing a sophisticated rewriting algorithm for.

	(dormant account) Assignee
	Comment 34 • 17 years ago

Most recent patch produced by the tool

	(dormant account) Assignee
	Comment 35 • 17 years ago

A lot of people seem to dislike 0 !=.

Should I change it to !!expr?

	Brendan Eich [:brendan] Reporter
	Comment 36 • 17 years ago

See comment 31 and use !! with my blessing.

/be

	Serge Gautherie (:sgautherie)
	Comment 37 • 17 years ago

Taras,
Are you still working on this ?

	(dormant account) Assignee
	Comment 38 • 17 years ago

Serge, 
I'm no longer actively working on this. I have a tool that scans code nightly and every once in a while I submit patches with corrections.

	Shawn Wilsher :sdwilsh
	Comment 39 • 17 years ago

(In reply to comment #38)
> Serge, 
> I'm no longer actively working on this. I have a tool that scans code nightly
> and every once in a while I submit patches with corrections.
Shouldn't this bug then be closed and any new issues that come up be filed as a new bug?

	Rob Campbell [:rc] (:robcee)
	Comment 40 • 17 years ago

it looks like Taras is using this as a tracking bug. Maybe prefix the summary with a [meta] tag to avoid confusion?

	(dormant account) Assignee
	Comment 41 • 16 years ago

Here are the latest errors outside of macros

	(dormant account) Assignee
	Comment 43 • 16 years ago

Here is a much nicer version of the error list

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 44 • 13 years ago

Michael, I assume we can close this bug and deps now?

	Michael Wu [:mwu]
	Comment 45 • 13 years ago

Bug 669808 finished fixing the remaining issues and switching to bool in bug 675553 ensures we shouldn't see this issue again.