explicit static_casts in Intermediate.cpp

[GoogleCodeExporter]

Compiling Intermediate.cpp on a Mac fails because there are implied 
static_casts.  These should be explicitly stated to prevent compile errors.  
Even casting from a size_t to an int needs to be explicit because of the 
truncating operation.  The following diff fixes the problem.  
https://bugs.webkit.org/show_bug.cgi?id=118815

Index: src/compiler/Intermediate.cpp
===================================================================
--- src/compiler/Intermediate.cpp   (revision 2426)
+++ src/compiler/Intermediate.cpp   (working copy)
@@ -1371,13 +1371,13 @@
             case EbtFloat:
                 switch (node->getType().getBasicType()) {
                     case EbtInt:
-                        
leftUnionArray[i].setFConst(static_cast<float>(node->getIConst(i)));
+                        
leftUnionArray[i].setFConst(static_cast<float>(node->getIConst(static_cast<int>(
i))));
                         break;
                     case EbtBool:
-                        
leftUnionArray[i].setFConst(static_cast<float>(node->getBConst(i)));
+                        
leftUnionArray[i].setFConst(static_cast<float>(node->getBConst(static_cast<bool>
(i))));
                         break;
                     case EbtFloat:
-                        
leftUnionArray[i].setFConst(static_cast<float>(node->getFConst(i)));
+                        
leftUnionArray[i].setFConst(static_cast<float>(node->getFConst(static_cast<float
>(i))));
                         break;
                     default:
                         infoSink.info.message(EPrefixInternalError, node->getLine(), "Cannot promote");
@@ -1387,13 +1387,13 @@
             case EbtInt:
                 switch (node->getType().getBasicType()) {
                     case EbtInt:
-                        
leftUnionArray[i].setIConst(static_cast<int>(node->getIConst(i)));
+                        
leftUnionArray[i].setIConst(static_cast<int>(node->getIConst(static_cast<int>(i)
)));
                         break;
                     case EbtBool:
-                        
leftUnionArray[i].setIConst(static_cast<int>(node->getBConst(i)));
+                        
leftUnionArray[i].setIConst(static_cast<int>(node->getBConst(static_cast<bool>(i
))));
                         break;
                     case EbtFloat:
-                        
leftUnionArray[i].setIConst(static_cast<int>(node->getFConst(i)));
+                        
leftUnionArray[i].setIConst(static_cast<int>(node->getFConst(static_cast<float>(
i))));
                         break;
                     default:
                         infoSink.info.message(EPrefixInternalError, node->getLine(), "Cannot promote");
@@ -1403,13 +1403,13 @@
             case EbtBool:
                 switch (node->getType().getBasicType()) {
                     case EbtInt:
-                        leftUnionArray[i].setBConst(node->getIConst(i) != 0);
+                        
leftUnionArray[i].setBConst(node->getIConst(static_cast<int>(i)) != 0);
                         break;
                     case EbtBool:
-                        leftUnionArray[i].setBConst(node->getBConst(i));
+                        
leftUnionArray[i].setBConst(node->getBConst(static_cast<bool>(i)));
                         break;
                     case EbtFloat:
-                        leftUnionArray[i].setBConst(node->getFConst(i) != 
0.0f);
+                        
leftUnionArray[i].setBConst(node->getFConst(static_cast<float>(i)) != 0.0f);
                         break;
                     default:
                         infoSink.info.message(EPrefixInternalError, node->getLine(), "Cannot promote");

Original issue reported on code.google.com by achriste...@gmail.com on 19 Jul 2013 at 7:32

[GoogleCodeExporter]

The 'i' variable iterates through the array of constants. It should not be cast 
to bool or float. Instead the issue here is that it's defined as a size_t while 
get*Const takes an int parameter.

Either 'i' should be cast to int, or get*Const should take a size_t parameter...

Original comment by nico...@transgaming.com on 30 Jul 2013 at 9:31

• Changed state: Accepted

[GoogleCodeExporter]

Yes, I added the wrong static_casts.  Maybe this would be a better change:

Index: src/compiler/intermediate.h
===================================================================
--- src/compiler/intermediate.h (revision 153502)
+++ src/compiler/intermediate.h (working copy)
@@ -369,9 +369,9 @@

     ConstantUnion* getUnionArrayPointer() const { return unionArrayPointer; }

-    int getIConst(int index) const { return unionArrayPointer ? 
unionArrayPointer[index].getIConst() : 0; }
-    float getFConst(int index) const { return unionArrayPointer ? 
unionArrayPointer[index].getFConst() : 0.0f; }
-    bool getBConst(int index) const { return unionArrayPointer ? 
unionArrayPointer[index].getBConst() : false; }
+    int getIConst(size_t index) const { return unionArrayPointer ? 
unionArrayPointer[index].getIConst() : 0; }
+    float getFConst(size_t index) const { return unionArrayPointer ? 
unionArrayPointer[index].getFConst() : 0.0f; }
+    bool getBConst(size_t index) const { return unionArrayPointer ? 
unionArrayPointer[index].getBConst() : false; }

     virtual TIntermConstantUnion* getAsConstantUnion()  { return this; }
     virtual void traverse(TIntermTraverser*);

Original comment by achriste...@gmail.com on 30 Jul 2013 at 11:59

[GoogleCodeExporter]

Original comment by nico...@transgaming.com on 12 Aug 2013 at 3:45

• Changed state: Started

[GoogleCodeExporter]

For review: https://codereview.appspot.com/13335045

Original comment by nico...@transgaming.com on 29 Aug 2013 at 4:02

• Changed state: Review

[GoogleCodeExporter]

It looks like this is finished.  Could this be committed?

Original comment by achriste...@gmail.com on 26 Sep 2013 at 3:51

[GoogleCodeExporter]

Original comment by c...@chromium.org on 16 Nov 2013 at 12:35

[GoogleCodeExporter]

Is there a reason why this has not been committed?  It has been finished and 
reviewed for months now.

Original comment by achriste...@gmail.com on 20 Nov 2013 at 4:27

[GoogleCodeExporter]

I just transitioned to Google. This patch was committed to the master branch 
yesterday. Should go into es3proto today. Sorry for the huge delay. Things 
should go much smoother now.

Original comment by nicolasc...@google.com on 20 Nov 2013 at 4:37

[GoogleCodeExporter]

FYI that's on the new Git repository at 
https://chromium.googlesource.com/angle/angle

Original comment by nicolasc...@google.com on 20 Nov 2013 at 5:53

[GoogleCodeExporter]

Fixed in If7be5f72e1037d92256edf45e1cfc7fedbf0b4bd and 
I4a16733e3e6aebf2939f8255ae2e4e8c5febc606

Original comment by c...@chromium.org on 20 Nov 2013 at 8:19

• Changed state: Fixed

[GoogleCodeExporter]

Those were the Change-Ids, let's see if the commit hashes provide a useful link:
9320a0c3060de8e81385094e37aad52001efa9ec
a621c2e458eab2b08751414729d4c092fb65738d

Original comment by c...@chromium.org on 20 Nov 2013 at 8:22

[GoogleCodeExporter]

Or maybe with an r in front:
r9320a0c3060de8e81385094e37aad52001efa9ec
ra621c2e458eab2b08751414729d4c092fb65738d

Original comment by c...@chromium.org on 20 Nov 2013 at 8:24

[GoogleCodeExporter]

I don't think the revision hash quicklinks are going to work with our source 
hosted on a different domain than our issue tracker. Full URLs are ugly but 
should work:

https://chromium.googlesource.com/angle/angle/+/9320a0c3060de8e81385094e37aad520
01efa9ec
https://chromium.googlesource.com/angle/angle/+/a621c2e458eab2b08751414729d4c092
fb65738d

Original comment by shannonw...@chromium.org on 20 Nov 2013 at 8:28