[webkit-reviews] review denied: [Bug 133395] Web Inspector: debugger should be able to show variable types : [Attachment 233394] patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Saam Barati <sbarati at apple.com>'s
request for review:
Bug 133395: Web Inspector: debugger should be able to show variable types
https://bugs.webkit.org/show_bug.cgi?id=133395

Attachment 233394: patch
https://bugs.webkit.org/attachment.cgi?id=233394&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=233394&action=review


Looks pretty good, just minor nits.

> Source/JavaScriptCore/jsc.cpp:942
> +
> +

Remove

> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:57
> +    //TODO: Should this functor be called with one operand?

Space between // and TODO.

And yes, this should keep the input alive.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1926
> +		   //TODO: handle other op.type here

Space between // and TODO

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1933
> +		       //TODO: implement

Ditto.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1969
> +	       //String name = nameForRegister(virtualRegister);
> +	       //printf("LINKING: op_profile_types_with_high_fidelity has
uniqueID:'%llu' and name:'%s', line:'%u', column:'%u'\n", globalID,
name.ascii().data(), location->line, location->column); 

Remove this.  Or, create a "static const bool verbose = false" and use that as
a guard.  Also, don't use printf() - use dataLog() instead.

> Source/JavaScriptCore/bytecode/TypeLocation.h:36
> +    enum HighFidelityGlobalIDFlags {
> +	   HighFidelityNeedsUniqueIDGeneration = -1,
> +	   HighFidelityNoGlobalIDExists = -2
> +    };

Unindent the enum.

> Source/JavaScriptCore/bytecode/TypeLocation.h:51
> +    //uint64_t globalVariableID;

Remove.

> Source/JavaScriptCore/bytecode/TypeLocation.h:53
> +    int64_t globalVariableID;
> +    uint64_t globalInstructionID;

Prepend "m_" to field names.

> Source/JavaScriptCore/bytecode/TypeLocation.h:54
> +    //size_t unlinkedByteCodeOffset;

Unique.

> Source/JavaScriptCore/bytecode/TypeLocation.h:57
> +    intptr_t sourceID;
> +    unsigned line;
> +    unsigned column;

Prepend "m_" to field names.

> Source/JavaScriptCore/bytecode/TypeLocation.h:58
> +    TypeSet* typeSet;

Use a std::unique_ptr<TypeSet> if this is owned by TypeLocation.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1120
> +    //op_profile_types_with_high_fidelity value_reg location hasGlobalID

Turn this comment into a sentence or remove it.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1123
> +    instructions().append(0);

You could add a comment at the end of this line saying what this zero means.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1129
> +    //instructions().append(offset);

Remove.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1884
> +	       //TODO: maybe emit expression info???

Space between // and TODO.  Only one ?.  Turn it into a sentence (i.e.
capitalize Maybe).

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:195
> +								    int
in_startLine, int in_startColumn, int in_endLine, int in_endColumn, String*
out_types) 

Bad indentation, you should make this line be only 4 spaces indented.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1471
> +    // put_to_scope_with_profile scope, id, value, ResolveModeAndType,
Structure, Operand, TypeLocation*

Same comment as before - turn this into a sentence or remove it.  For example,
you could make this say:

// The format of this instruction is: put_to_scope_with_profile scope, id,
value, ResolveModeAndType, Structure, Operand, TypeLocation*

> Source/JavaScriptCore/runtime/HighFidelityTypeProfiler.h:2
> + * Copyright (C) 2008, 2014 Apple Inc. All Rights Reserved.

Only say 2014.

> Source/JavaScriptCore/runtime/SymbolTable.h:419
> +	   //use -1 as a flag to indicate we need to produce a unique ID
because we need the VM to do this. 
> +	   //When linking, we can produce this unique ID as we have access to
VM

Complete sentences in comments, capitalize the first word in the sentence, and
a space between // and use, and between // and When.

> Source/JavaScriptCore/runtime/SymbolTable.h:472
> -    
> +

Revert.

> Source/JavaScriptCore/runtime/SymbolTable.h:485
> -    
> +

Revert.

> Source/JavaScriptCore/runtime/SymbolTable.h:500
> -	   
> +

Revert.

> Source/JavaScriptCore/runtime/SymbolTable.h:507
> -    
> +

Revert.

> Source/JavaScriptCore/runtime/SymbolTable.h:522
> -    
> +

Revert.

> Source/JavaScriptCore/runtime/SymbolTable.h:527
> -    
> +

Revert.

> Source/JavaScriptCore/runtime/TypeSet.cpp:63
> +    else if (v.isGetterSetter())
> +	   ret = TypeGetterSetter;

Remove.

> Source/JavaScriptCore/runtime/TypeSet.cpp:65
> +    //else if (v.isCustomGetterSetter())
> +    //    ret = TypeCustomGetterSetter;

Remove.

> Source/JavaScriptCore/runtime/TypeSet.cpp:74
> +//structure may be null

Space between // and structure, turn it into a sentence.  Or remove it.

> Source/JavaScriptCore/runtime/TypeSet.cpp:139
> +    if (m_seenTypes & TypeGetterSetter)
> +	    seen.append("GetterSetter ");
> +    if (m_seenTypes & TypeCustomGetterSetter)
> +	    seen.append("CustomGetterSetter ");
> +    //if (m_seenTypes & TypeObject)
> +    //     seen.append("Object ");

Remove.

> Source/JavaScriptCore/runtime/TypeSet.h:53
> +    class JSValue;
> +    class Structure;
> +
> +    enum RuntimeType {
> +	   TypeNothing		  = 0x0,
> +	   TypeFunction 	  = 0x1,
> +	   TypeUndefined	  = 0x2,
> +	   TypeNull		  = 0x4,
> +	   TypeBoolean		  = 0x8,
> +	   TypeMachineInt	  = 0x10,
> +	   TypeNumber		  = 0x20,
> +	   TypeString		  = 0x40,
> +	   TypePrimitive	  = 0x80,
> +	   TypeGetterSetter	  = 0x100,
> +	   TypeCustomGetterSetter = 0x200,
> +	   TypeObject		  = 0x400,
> +    };
> +

Unindent.

> Source/JavaScriptCore/runtime/TypeSet.h:54
> +

Remove this blank line.

> Source/JavaScriptCore/runtime/TypeSet.h:62
> +

Remove this blank line.

> Source/JavaScriptCore/runtime/VM.cpp:335
> +    //TODO: conditionally allocate all this stuff based on whether the
inspector is running

Space between // and TODO.

> Source/JavaScriptCore/runtime/VM.cpp:943
> +    //printf("Getting type for variable given sourceID:'%ld' line:'%u'\n",
sourceID, startLine);

Remove or guard with a verbose flag.

> Source/JavaScriptCore/runtime/VM.cpp:971
> +	   printf("[Line, Column]::[%u, %u] Local:'%s' Global:'%s'\n",
location->line, location->column, 
> +						 
profiler->getLocalTypesForVariableInRange(location->line, location->column,
location->line, location->column, "", location->sourceID).ascii().data(),
> +						 
profiler->getGlobalTypesForVariableInRange(location->line, location->column,
location->line, location->column, "", location->sourceID).ascii().data());

Use dataLog().