676759 - Define PR_TRUE/FALSE as true/false when in c++ code

Status: RESOLVED WONTFIX

(NSPR :: NSPR, defect)

Description

[Michael Wu [:mwu]]

Attachment: Define PR_TRUE/FALSE as true/false when in c++

Needed to help the compiler disambiguate in certain cases.

Comment 1

[(not currently active) Ted Mielczarek]

I'm not sure about this. This will break binary compatibility, won't it?

Comment 2

[Wan-Teh Chang]

Michael: thank you for the patch.  You didn't describe the benefit
of this change, other than the terse "needed to help the compiler
disambiguate in certain cases".  I also read bug 675553, which this
bug blocks, but didn't get more useful info.  Can you give an example
of how this patch would help the compiler disambiguate?

Re: Ted'd concern about breaking binary compatibility: defining
PRBool as bool instead of PRIntn is more likely to break binary
compatibility.  But I think PR_TRUE and PR_FALSE should be values
of the type PRBool.  That seems more consistent.

Comment 3

[Michael Wu [:mwu]]

(In reply to Wan-Teh Chang from comment #2)
> Michael: thank you for the patch.  You didn't describe the benefit
> of this change, other than the terse "needed to help the compiler
> disambiguate in certain cases".  I also read bug 675553, which this
> bug blocks, but didn't get more useful info.  Can you give an example
> of how this patch would help the compiler disambiguate?
> 
Given a choice between two functions with the same name: eg.

void foo(int, bool)
and
void foo(int, int)

using true/false of the proper type will let the compiler choose the right one. 

> Re: Ted'd concern about breaking binary compatibility: defining
> PRBool as bool instead of PRIntn is more likely to break binary
> compatibility.  But I think PR_TRUE and PR_FALSE should be values
> of the type PRBool.  That seems more consistent.

Unfortunately, PRBool is just an int and wouldn't allow us to do the above.

Comment 4

[Wan-Teh Chang]

Comment on attachment 550961 [details] [diff] [review]
Define PR_TRUE/FALSE as true/false when in c++

To call the function
    void foo(int, bool)
one should pass true/false as the second argument.  If the
problem is that some Mozilla code is passing PR_TRUE/PR_FALSE
as the second argument, the correct fix is to change the code
to pass true/false.

Do I understand the problem correctly?  Are you proposing
this patch as a convenient solution to the problem (a small
change to a single file vs. changing several files)?

I checked the Windows <windef.h> header.  It defines the BOOL
type and the TRUE/FALSE macros the same way NSPR defines PRBool
and PR_TRUE/PR_FALSE:

#ifndef FALSE
#define FALSE               0
#endif

#ifndef TRUE
#define TRUE                1
#endif

typedef int                 BOOL;

Comment 5

[Michael Wu [:mwu]]

(In reply to Wan-Teh Chang from comment #4)
> Do I understand the problem correctly?  Are you proposing
> this patch as a convenient solution to the problem (a small
> change to a single file vs. changing several files)?
> 

Yeah. Nevermind about this patch. I'll just change every caller that relies on this. It'll discourage people from using PR_TRUE/PR_FALSE in the future.

Comment 6

[Wan-Teh Chang]

Michael: I looked at all the bugs that bug 675553 depends on.
I now have a better idea of the general problem you're trying
to solve.

Using multiple boolean types is a natural outcome of reusing
third-party code.  I wonder if there is a place where we can
document the following guidelines.

1. Use 'bool' in C++ code unless you are calling a third-party
library that uses its own boolean type.

2. Use the true and false constants that match the boolean type.

Do you agree with these guidelines?

Is it possible to use clang to enforce the second guideline?

Comment 7

[Michael Wu [:mwu]]

(In reply to Wan-Teh Chang from comment #6)
> Michael: I looked at all the bugs that bug 675553 depends on.
> I now have a better idea of the general problem you're trying
> to solve.
> 
> Using multiple boolean types is a natural outcome of reusing
> third-party code.  I wonder if there is a place where we can
> document the following guidelines.
> 
> 1. Use 'bool' in C++ code unless you are calling a third-party
> library that uses its own boolean type.
> 
> 2. Use the true and false constants that match the boolean type.
> 
> Do you agree with these guidelines?
> 
> Is it possible to use clang to enforce the second guideline?

Yes and yes. However, I'm trying to avoid changing more code than I need in order to avoid destroying hg annotate/blame too much. We may have PR_(TRUE|FALSE) assignments to bool for a bit longer until it starts to naturally fade out and we can just manually stomp it out of code that nobody will touch.

Writing a plugin to do this analysis is possible, though it may not happen since mismatches generally don't cause real bugs.