dart2js OldEmitter: Small cleanups in output.

pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart

Index: pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart
diff --git a/pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart b/pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart
index 29179732ad5ca4f84339c73d26e2ad63d7b6818b..81abf5d55f43ca333c513d27273f1bd76e382209 100644
--- a/pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart
+++ b/pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart
@@ -231,7 +231,6 @@ jsAst.Expression getReflectionDataParser(OldEmitter oldEmitter,
            requiredParameterCount + optionalParameterCount != funcs[0].length;
     var functionTypeIndex = ${readFunctionType("array", "2")};
     var unmangledNameIndex = $unmangledNameIndex;
-    var isReflectable = array.length > unmangledNameIndex;

[ahe, 2015/01/05 14:42:25]

Decoding "array" is now spread out in the code. The variable was declared here
to make it easier to read the code.

[floitsch, 2015/01/05 14:52:53]

I don't think there is a good solution:
- we don't need the isReflectable when reflection isn't enabled.
- we don't want to put that line into an "if (#x)".
- we don't want to move the predicate computation away.

These are mutually exclusive. :(

I'm ok either way (although I like the 'if (#x)' the least.

[ahe, 2015/01/05 15:05:24]

I think I know what your answer is going to be, but to be explicit about it:
why?
I wasn't clear: I don't see this as a big deal. I just wanted to bring attention
to why I introduced the variable in the first place.

[floitsch, 2015/01/05 15:07:43]

Mostly personal preference. I don't like "if"s all over the place. Especially if
two of them have to work together.

 
     if (getterStubName) {
       f = tearOff(funcs, array, isStatic, name, isIntercepted);
@@ -246,32 +245,37 @@ jsAst.Expression getReflectionDataParser(OldEmitter oldEmitter,
       f.\$callName = null;
       if (isIntercepted) #interceptedNames[getterStubName] = true;
     }
-    if (isReflectable) {
-      for (var i = 0; i < funcs.length; i++) {
-        funcs[i].$reflectableField = 1;
-        funcs[i].$reflectionInfoField = array;
-      }
-      var mangledNames = isStatic ? #mangledGlobalNames : #mangledNames;
-      var unmangledName = ${readString("array", "unmangledNameIndex")};
-      // The function is either a getter, a setter, or a method.
-      // If it is a method, it might also have a tear-off closure.
-      // The unmangledName is the same as the getter-name.
-      var reflectionName = unmangledName;
-      if (getterStubName) mangledNames[getterStubName] = reflectionName;
-      if (isSetter) {
-        reflectionName += "=";
-      } else if (!isGetter) {
-        reflectionName += ":" + requiredParameterCount +
-          ":" + optionalParameterCount;
+
+    if (#isReflectionEnabled) {
+      var isReflectable = array.length > unmangledNameIndex;
+      if (isReflectable) {

[ahe, 2015/01/05 14:42:25]

I would write:

if (!isReflectable) continue;

This way the indentation doesn't change and the code is easier to read.

Alternatively, I would remove the isReflectable variable altogether:

if (#isReflectionEnabled && isReflectable) {

[floitsch, 2015/01/05 14:52:53]

Adding a "continue" here assumes too much:
a- we are in a loop (not even visible in the review)
b- no code after this if is executed. Currently that's the case, but it invites
subtle bugs (only happening during reflection) for future code changes.

However: we could not intend for the "if (#xxx)" condition, since that's more of
an #ifdef anyways.

[ahe, 2015/01/05 15:05:24]

I don't know what I was thinking.

+        for (var i = 0; i < funcs.length; i++) {
+          funcs[i].$reflectableField = 1;
+          funcs[i].$reflectionInfoField = array;
+        }
+        var mangledNames = isStatic ? #mangledGlobalNames : #mangledNames;
+        var unmangledName = ${readString("array", "unmangledNameIndex")};
+        // The function is either a getter, a setter, or a method.
+        // If it is a method, it might also have a tear-off closure.
+        // The unmangledName is the same as the getter-name.
+        var reflectionName = unmangledName;
+        if (getterStubName) mangledNames[getterStubName] = reflectionName;
+        if (isSetter) {
+          reflectionName += "=";
+        } else if (!isGetter) {
+          reflectionName += ":" + requiredParameterCount +
+            ":" + optionalParameterCount;
+        }
+        mangledNames[name] = reflectionName;
+        funcs[0].$reflectionNameField = reflectionName;
+        funcs[0].$metadataIndexField = unmangledNameIndex + 1;
+        if (optionalParameterCount) descriptor[unmangledName + "*"] = funcs[0];
       }
-      mangledNames[name] = reflectionName;
-      funcs[0].$reflectionNameField = reflectionName;
-      funcs[0].$metadataIndexField = unmangledNameIndex + 1;
-      if (optionalParameterCount) descriptor[unmangledName + "*"] = funcs[0];
     }
   }
 ''', {'globalFunctions' : globalFunctionsAccess,
       'interceptedNames': interceptedNamesAccess,
+      'isReflectionEnabled': compiler.mirrorsLibrary != null,
       'mangledGlobalNames': mangledGlobalNamesAccess,
       'mangledNames': mangledNamesAccess});