[swift-dev] [Draft Proposal] Refactor SILParser::parseSILInstruction

[Jordan Rose]

Hey, Sergey. It definitely makes sense to refactor this, but I don't think putting methods on SILInstruction is the way to go about it. There's no reason most clients of SIL need to know anything about parsing; from a separation-of-concerns perspective it belongs in a separate header and very likely a separate component (i.e. not lib/SIL/).

If this were Swift, we could use an extension, but alas. :-)

That said, I'm not a heavy user of SIL, so someone else from the Swift team with more of a stake should comment.

Jordan


> On Jan 13, 2016, at 13:27, Sergey Bolshedvorsky via swift-dev <swift-dev at swift.org> wrote:
> 
> Hi everyone,
> 
> I’m interested in taking SR-340 <https://bugs.swift.org/browse/SR-340> issue:
> "parseSILInstruction is horrible and makes me cry every time I see it. It is a method that is ~1900 lines with a huge switch in it. We should refactor it into a visitor structure. In fact it is large enough that we should consider moving it into its own file if it is possible."
> 
> Here is the first draft of the myl idea, how it could be approached:
> 
> 1. We will define an abstract method on ValueBase class:
> 
>     class ValueBase
>     {
>     public:
>         virtual void parse(class SILParseInstruction*) = 0;
>     };
>     
> 
> 2. Each of the SIL instruction classes will override this method and will provide the following implementation:
>     
>     class SILArgument: public ValueBase
>     {
>     public:
>         /*virtual*/void parse(SILParseInstruction*);
>     };
>     void SILArgument::parse(SILParseInstruction *i) {
>         i->parseInstruction(this);
>     }
>     
>     class PartialApplyInst: public ValueBase
>     {
>     public:
>         /*virtual*/void parse(SILParseInstruction*);
>     };
>     void PartialApplyInst::parse(SILParseInstruction *i)
>     {
>         i->parseInstruction(this);
>     }
>     
> 
> 3. We will define an abstract class for the callbacks from each of the SIL instruction classes
> 
>     class SILParseInstruction
>     {
>     public:
>         virtual void parseInstruction(SILArgument*) = 0;
>         virtual void parseInstruction(PartialApplyInst*) = 0;
>     };
> 
> 4. SILParser will implement these callbacks with the actual handling operations for each instruction. 
>     
>     class SILParser: public SILParseInstruction
>     {
>     public:
>         /*virtual*/void parseInstruction(SILArgument *r)
>         {
>             // Parse StringLiteralInst instruction
>         }
>         /*virtual*/void parseInstruction(PartialApplyInst *b)
>         {
>             // Parse PartialApplyInst instruction
>         }
>     };
>     
> 5. The huge switch statement will be replaced with a single call:
> 	Opcode->parse(this);
> 
> What are your thoughts?
> 
> Sergey
> 
> _______________________________________________
> swift-evolution mailing list
> swift-evolution at swift.org <mailto:swift-evolution at swift.org>
> https://lists.swift.org/mailman/listinfo/swift-evolution <https://lists.swift.org/mailman/listinfo/swift-evolution>
> 
> 
> 
> _______________________________________________
> swift-dev mailing list
> swift-dev at swift.org
> https://lists.swift.org/mailman/listinfo/swift-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-dev/attachments/20160113/0439e733/attachment.html>