Last Comment Bug 699041 - Remove MBool / MB_TRUE / MB_FALSE
: Remove MBool / MB_TRUE / MB_FALSE
Status: RESOLVED FIXED
[good first bug][mentor=Ms2ger]
:
Product: Core
Classification: Components
Component: XSLT (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Abhishek Singh
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-02 07:50 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2011-11-11 02:19 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch_v (74.27 KB, patch)
2011-11-07 05:24 PST, Abhishek Singh
jonas: review-
Details | Diff | Splinter Review
Patch_v1 (74.27 KB, patch)
2011-11-07 22:54 PST, Abhishek Singh
Ms2ger: review-
Details | Diff | Splinter Review
Patch_A (74.33 KB, patch)
2011-11-09 05:26 PST, Abhishek Singh
jonas: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
Patch_A1 (74.31 KB, patch)
2011-11-10 04:12 PST, Abhishek Singh
no flags Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2011-11-02 07:50:27 PDT
They can simply be replaced with bool/true/false.
Comment 1 Abhishek Singh 2011-11-07 05:24:19 PST
Created attachment 572440 [details] [diff] [review]
Patch_v
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2011-11-07 09:32:05 PST
Comment on attachment 572440 [details] [diff] [review]
Patch_v

Review of attachment 572440 [details] [diff] [review]:
-----------------------------------------------------------------

it seems like there are some big whitespace issues in this patch. Looks good otherwise though, but that needs to be fixed before this can land.

::: content/xslt/src/base/txCore.h
@@ +108,5 @@
>  
>  // XXX These should go away eventually.
>  #define TxObject txObject
>  typedef txDouble Double;
> +typedef boolbool;

This line looks all wrong

@@ +113,3 @@
>  
> +
> +

And just remove these lines rather than adding to empty lines.

::: content/xslt/src/base/txExpandedNameMap.h
@@ +94,5 @@
>                mCurrentPos(PRUint32(-1))
>          {
>          }
>  
> +       bool next()

Whitespace here is wrong.

::: content/xslt/src/base/txList.cpp
@@ +281,3 @@
>  **/
> +bool txListIterator::hasNext() {
> +   bool hasNext = false;

And here.

@@ +296,3 @@
>  **/
> +bool txListIterator::hasPrevious() {
> +   bool hasPrevious = false;

And here :(
Comment 3 Abhishek Singh 2011-11-07 22:54:18 PST
Created attachment 572745 [details] [diff] [review]
Patch_v1
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2011-11-07 23:22:21 PST
Comment on attachment 572745 [details] [diff] [review]
Patch_v1

>--- a/content/xslt/src/base/txCore.h	Sat Nov 05 23:41:16 2011 +0200
>+++ b/content/xslt/src/base/txCore.h	Tue Nov 08 17:52:26 2011 +0530
>-typedef bool MBool;
>+typedef bool bool;

Remove this line

>--- a/content/xslt/src/base/txList.h	Sat Nov 05 23:41:16 2011 +0200
>+++ b/content/xslt/src/base/txList.h	Tue Nov 08 17:52:26 2011 +0530

Indentation is off in this file.

>--- a/content/xslt/src/xml/txDOM.h	Sat Nov 05 23:41:16 2011 +0200
>+++ b/content/xslt/src/xml/txDOM.h	Tue Nov 08 17:52:26 2011 +0530
>-    virtual MBool hasChildNodes() const = 0;
>+    virtualbool hasChildNodes() const = 0;

And this won't work.

>-    MBool hasChildNodes() const;
>+   bool hasChildNodes() const;

Indentation.

Same comments elsewhere in the patch.
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2011-11-08 01:37:50 PST
It appears that you've replaced " MBool" with "bool" in both versions of this patch which is leading to the whitespace issue. Please just replace "MBool" with "bool" in order to fix it. This is happening everywhere, not just in the places commented on so far.
Comment 6 Abhishek Singh 2011-11-09 05:26:04 PST
Created attachment 573170 [details] [diff] [review]
Patch_A
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2011-11-09 13:24:12 PST
Comment on attachment 573170 [details] [diff] [review]
Patch_A

>--- a/content/xslt/src/base/txCore.h	Mon Nov 07 07:16:24 2011 -0500
>+++ b/content/xslt/src/base/txCore.h	Thu Nov 10 00:24:50 2011 +0530
>@@ -104,15 +104,13 @@ public:
>      * represent a double, NaN will be returned.
>      */
>     static double toDouble(const nsAString& aStr);
> };
> 
> // XXX These should go away eventually.
> #define TxObject txObject
> typedef txDouble Double;
>-typedef bool MBool;
>+typedef bool bool;

This typedef still needs to go. Looks good otherwise.

I can't review this, though. Jonas?
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2011-11-09 17:17:03 PST
Comment on attachment 573170 [details] [diff] [review]
Patch_A

Review of attachment 573170 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that fixed.

::: content/xslt/src/base/txCore.h
@@ +108,5 @@
>  
>  // XXX These should go away eventually.
>  #define TxObject txObject
>  typedef txDouble Double;
> +typedef bool bool;

Remove this
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2011-11-10 03:23:43 PST
Abhishek, if you could generate patch with that line removed, and as described at <https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F>, that would be really helpful.

Thanks for taking this bug!
Comment 10 Abhishek Singh 2011-11-10 04:12:55 PST
Created attachment 573479 [details] [diff] [review]
Patch_A1
Comment 11 Dão Gottwald [:dao] 2011-11-10 05:58:52 PST
patch failed to apply in content/xslt/src/xpath/txExprParser.cpp
Comment 13 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-11-11 02:19:12 PST
https://hg.mozilla.org/mozilla-central/rev/33d34da275ed

Note You need to log in before you can comment on or make changes to this bug.