dart2js OldEmitter: Small cleanups in output.

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

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 part of dart2js.js_emitter;
 
 // TODO(ahe): Share these with js_helper.dart.
 const FUNCTION_INDEX = 0;
 const NAME_INDEX = 1;
 const CALL_NAME_INDEX = 2;
@@ skipping 213 matching lines @@
     var isAccessor = (requiredParameterInfo & 1) === 1;
     var isSetter = requiredParameterInfo === 3;
     var isGetter = requiredParameterInfo === 1;
     var optionalParameterInfo = ${readInt("array", "1")};
     var optionalParameterCount = optionalParameterInfo >> 1;
     var optionalParametersAreNamed = (optionalParameterInfo & 1) === 1;
     var isIntercepted =
            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);
       descriptor[name].\$getter = f;
       f.\$getterStub = true;
       // Used to create an isolate using spawnFunction.
       if (isStatic) #globalFunctions[name] = f;
       originalDescriptor[getterStubName] = descriptor[getterStubName] = f;
       funcs.push(f);
       if (getterStubName) functions.push(getterStubName);
       f.\$stubName = getterStubName;
       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;
+
+    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];
       }
-      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];
     }
   }
 ''', {'globalFunctions' : globalFunctionsAccess,
       'interceptedNames': interceptedNamesAccess,
+      'isReflectionEnabled': compiler.mirrorsLibrary != null,
       'mangledGlobalNames': mangledGlobalNamesAccess,
       'mangledNames': mangledNamesAccess});
 
   List<jsAst.Statement> tearOffCode = buildTearOffCode(backend);
 
   jsAst.Statement init = js.statement('''{
   var functionCounter = 0;
   if (!#libraries) #libraries = [];
   if (!#mangledNames) #mangledNames = map();
   if (!#mangledGlobalNames) #mangledGlobalNames = map();
@@ skipping 274 matching lines @@
 (function() {
   var result = $array[$index];
   if ($check) {
     throw new Error(
         name + ": expected value of type \'$type\' at index " + ($index) +
         " but got " + (typeof result));
   }
   return result;
 })()''';
 }