Interface Dispatch #1848
Interface Dispatch #1848
Conversation
|
|
||
| if (entrypoint != null) | ||
| { | ||
| OutputMainMethodStub(entrypoint); | ||
| } | ||
| // Stub for main method |
There was a problem hiding this comment.
I have moved this code to separate OutputMainMethodStub to make it better factored. Could you please keep it in separate function?
| Out.Write(sb.ToString()); | ||
| sb.Clear(); | ||
|
|
||
| Out.Write(methodImplementations.ToString()); |
There was a problem hiding this comment.
Could you please keep the refactoring that I have done - that avoids building one huge string out of all method implementations?
It is a lot cycles wasted by copying all method implementations into a single string. It is better to write the individual implementations into the stream directly.
| Out.Write(methodTables.ToString()); | ||
| Out.Write(optionalFields.ToString()); | ||
| definitions.Append(forwardDefinitions.ToString()); | ||
| forwardDefinitions.Clear(); |
There was a problem hiding this comment.
These Clear calls are unnecessary because of the builder is not reused.
| return sb.ToString(); | ||
| } | ||
|
|
||
| private String GetInterfaceDispatchHelper() | ||
| { |
There was a problem hiding this comment.
This does not seem to be used. Is this left-over method?
A-And
force-pushed
the
InterfDispatch
branch
2 times, most recently
from
71eadf3
to
954e516
Compare
| @@ -900,7 +889,7 @@ private String GetCodeForNodeStruct(List<NodeDataSection> nodeDataDivs, Dependen | |||
| return nodeStructDecl.ToString(); | |||
| } | |||
|
|
|||
| private void AppendFormattedByteArray(CppGenerationBuffer sb, byte[] array, int startIndex, int endIndex) | |||
| private void GetFormattedByteArray(byte[] array, int startIndex, int endIndex, CppGenerationBuffer sb) | |||
There was a problem hiding this comment.
It would be better to keep the original name - it better expresses what the method does.
Also, you can make the method static while you are on it.
| @@ -1239,8 +1285,11 @@ public void OutputCode(IEnumerable<DependencyNode> nodes, MethodDesc entrypoint, | |||
| _threadStatics.Indent(); | |||
| _gcThreadStatics = new CppGenerationBuffer(); | |||
| _gcThreadStatics.Indent(); | |||
| var methodImplementations = new CppGenerationBuffer(); | |||
| Out.Write(sb.ToString()); | ||
| } | ||
|
|
||
| public void OutputCode(IEnumerable<DependencyNode> nodes, MethodDesc entrypoint, NodeFactory factory) | ||
| { | ||
| var sb = new CppGenerationBuffer(); |
There was a problem hiding this comment.
Unused - it looks like all changes in this function should be undone.
A-And
force-pushed
the
InterfDispatch
branch
2 times, most recently
from
d3e0f69
to
de8ff17
Compare
|
@jkotas @MichalStrehovsky Cleaned up. Could you take another look? (force pushing to fix build errors caused a commit to disappear, so the comments on the diffs are not current) |
| private void AddWellKnownTypes(DependencyAnalysisFramework.DependencyAnalyzerBase<NodeFactory> graph) | ||
| { | ||
|
|
||
| AddWellKnownType(WellKnownType.Void, graph); |
There was a problem hiding this comment.
Did you look into finding a way to not duplicate the list from _wellKnownTypeNodes from CppWriter in here? I dislike that we're setting a precedent here to root a bunch of things without explanation.
(I assume _wellKnownTypeNodes is the reason why we need this rooting in the first place.)
There was a problem hiding this comment.
_wellKnownTypeNodes was a workaround for not rooting well-known types before. All of the well-known types are not always constructed, so their VTables isn't emitted (which we need for the C++ to compile). Instead of manually rooting another approach might be manually requesting a constructed type node?
There was a problem hiding this comment.
Can you put a TODO on line 56 (the empty line) to unify the list here with the list in CppWriter? This should be in a central location but I don't have time to think about where.
| private void OutputTypeNode(IEETypeNode typeNode, NodeFactory factory, CppGenerationBuffer forwardDefinitions, CppGenerationBuffer typeDefinitions, CppGenerationBuffer methodTable) | ||
| { | ||
| if (_emittedTypes == null) | ||
| { | ||
| _emittedTypes = new HashSet<TypeDesc>(); | ||
| } | ||
| if (!_emittedTypes.Contains(typeNode.Type)) |
There was a problem hiding this comment.
This check is redundant - done again a few lines later. (Also, it would look better to have the early out as before so that most of the function does not need to be indented.)
| @@ -1265,9 +1320,9 @@ public void OutputCode(IEnumerable<DependencyNode> nodes, MethodDesc entrypoint, | |||
| if (entrypoint != null) | |||
| { | |||
| OutputMainMethodStub(entrypoint); | |||
| Out.Dispose(); | ||
| } | ||
| } | ||
| } | ||
| } |
| @@ -1021,7 +1076,7 @@ private void ImportCall(ILOpcode opcode, int token) | |||
|
|
|||
| PushExpression(StackValueKind.NativeInt, sb.ToString()); | |||
| } | |||
| } | |||
| } | |||
| @@ -593,7 +593,7 @@ public void AttachToDependencyGraph(DependencyAnalysisFramework.DependencyAnalyz | |||
|
|
|||
| _compilationModuleGroup.AddCompilationRoots(new RootingServiceProvider(graph, this)); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Nit: Unnecessary white space change.
Interface Dispatch implementation Interface Dispatch Interface Dispatch
A-And
force-pushed
the
InterfDispatch
branch
from
c678385
to
49b1246
Compare
Interface Dispatch implementation