Renaming SILSuccessor -> SILCFGEdge


(Michael Gottesman) #1

Hey everyone.

I am currently doing some small fixes to SILSuccessor (adding some comments and fixing some issues exposed by LLVM upstream). As I read the code it became pretty apparent that the name is a misnomer... SILSuccessor is not just representing a successor, rather it is representing a whole CFG edge. This can be seen in how SILSuccessor is used to iterate over the predecessors of the block.

With that in mind, I would like to rename SILSuccessor to SILCFGEdge. It will make it much clearer without knowing any context what this data structure is used for.

Any objections, disagreements, flames, etc?
Michael


(John McCall) #2

It seems a little unnecessary to me. The successor relationship is an edge, and all the edges of the local CFG are successor relationships. I guess it looks a little funny that the edges into a block are represented by "successors", but I think when you think about it it makes sense.

"SILCFGEdge" is also not a very attractive name because of the two abbreviations. If you had a nice alternative to "CFGEdge" that was less biased to the beginning/end (like Successor/Predecessor are), I probably wouldn't object.

John.

···

On Apr 26, 2017, at 4:24 PM, Michael Gottesman via swift-dev <swift-dev@swift.org> wrote:
Hey everyone.

I am currently doing some small fixes to SILSuccessor (adding some comments and fixing some issues exposed by LLVM upstream). As I read the code it became pretty apparent that the name is a misnomer... SILSuccessor is not just representing a successor, rather it is representing a whole CFG edge. This can be seen in how SILSuccessor is used to iterate over the predecessors of the block.

With that in mind, I would like to rename SILSuccessor to SILCFGEdge. It will make it much clearer without knowing any context what this data structure is used for.

Any objections, disagreements, flames, etc?


(Andrew Trick) #3

Please call this SILBasicBlockEdge. CFG isn’t used anywhere in SIL type names.The guiding principle is to minimize the variation names used for the same entity.

-Andy

···

On Apr 26, 2017, at 1:24 PM, Michael Gottesman via swift-dev <swift-dev@swift.org> wrote:

Hey everyone.

I am currently doing some small fixes to SILSuccessor (adding some comments and fixing some issues exposed by LLVM upstream). As I read the code it became pretty apparent that the name is a misnomer... SILSuccessor is not just representing a successor, rather it is representing a whole CFG edge. This can be seen in how SILSuccessor is used to iterate over the predecessors of the block.

With that in mind, I would like to rename SILSuccessor to SILCFGEdge. It will make it much clearer without knowing any context what this data structure is used for.

Any objections, disagreements, flames, etc?
Michael


(Michael Gottesman) #4

Hey everyone.

I am currently doing some small fixes to SILSuccessor (adding some comments and fixing some issues exposed by LLVM upstream). As I read the code it became pretty apparent that the name is a misnomer... SILSuccessor is not just representing a successor, rather it is representing a whole CFG edge. This can be seen in how SILSuccessor is used to iterate over the predecessors of the block.

With that in mind, I would like to rename SILSuccessor to SILCFGEdge. It will make it much clearer without knowing any context what this data structure is used for.

Any objections, disagreements, flames, etc?

It seems a little unnecessary to me. The successor relationship is an edge, and all the edges of the local CFG are successor relationships. I guess it looks a little funny that the edges into a block are represented by "successors", but I think when you think about it it makes sense.

IMO this is more of an issue than something "looking funny". Using code named "successor" to look up the "predecessors" of a block is misleading and results in unnecessary cognitive overhead for the reader who has to "think about it" for it to make sense.

"SILCFGEdge" is also not a very attractive name because of the two abbreviations. If you had a nice alternative to "CFGEdge" that was less biased to the beginning/end (like Successor/Predecessor are), I probably wouldn't object.

Ok. Maybe SILControlFlowEdge?

···

On Apr 26, 2017, at 1:44 PM, John McCall <rjmccall@apple.com> wrote:

On Apr 26, 2017, at 4:24 PM, Michael Gottesman via swift-dev <swift-dev@swift.org> wrote:

John.


(Bob Wilson) #5

I can see your point here, but a big renaming change like this is disruptive. With all the other things we’re trying to get done now, is this really the right time to pay the cost of that?

···

On Apr 26, 2017, at 3:11 PM, Michael Gottesman via swift-dev <swift-dev@swift.org> wrote:

On Apr 26, 2017, at 1:44 PM, John McCall <rjmccall@apple.com> wrote:

On Apr 26, 2017, at 4:24 PM, Michael Gottesman via swift-dev <swift-dev@swift.org> wrote:
Hey everyone.

I am currently doing some small fixes to SILSuccessor (adding some comments and fixing some issues exposed by LLVM upstream). As I read the code it became pretty apparent that the name is a misnomer... SILSuccessor is not just representing a successor, rather it is representing a whole CFG edge. This can be seen in how SILSuccessor is used to iterate over the predecessors of the block.

With that in mind, I would like to rename SILSuccessor to SILCFGEdge. It will make it much clearer without knowing any context what this data structure is used for.

Any objections, disagreements, flames, etc?

It seems a little unnecessary to me. The successor relationship is an edge, and all the edges of the local CFG are successor relationships. I guess it looks a little funny that the edges into a block are represented by "successors", but I think when you think about it it makes sense.

IMO this is more of an issue than something "looking funny". Using code named "successor" to look up the "predecessors" of a block is misleading and results in unnecessary cognitive overhead for the reader who has to "think about it" for it to make sense.

"SILCFGEdge" is also not a very attractive name because of the two abbreviations. If you had a nice alternative to "CFGEdge" that was less biased to the beginning/end (like Successor/Predecessor are), I probably wouldn't object.

Ok. Maybe SILControlFlowEdge?


(Michael Gottesman) #6

I am not planning on doing it now. What I /am/ trying to do is get some sort of consensus so when I find time to do it I can just do it.

Michael

···

On Apr 26, 2017, at 4:20 PM, Bob Wilson <bob.wilson@apple.com> wrote:

On Apr 26, 2017, at 3:11 PM, Michael Gottesman via swift-dev <swift-dev@swift.org> wrote:

On Apr 26, 2017, at 1:44 PM, John McCall <rjmccall@apple.com> wrote:

On Apr 26, 2017, at 4:24 PM, Michael Gottesman via swift-dev <swift-dev@swift.org> wrote:
Hey everyone.

I am currently doing some small fixes to SILSuccessor (adding some comments and fixing some issues exposed by LLVM upstream). As I read the code it became pretty apparent that the name is a misnomer... SILSuccessor is not just representing a successor, rather it is representing a whole CFG edge. This can be seen in how SILSuccessor is used to iterate over the predecessors of the block.

With that in mind, I would like to rename SILSuccessor to SILCFGEdge. It will make it much clearer without knowing any context what this data structure is used for.

Any objections, disagreements, flames, etc?

It seems a little unnecessary to me. The successor relationship is an edge, and all the edges of the local CFG are successor relationships. I guess it looks a little funny that the edges into a block are represented by "successors", but I think when you think about it it makes sense.

IMO this is more of an issue than something "looking funny". Using code named "successor" to look up the "predecessors" of a block is misleading and results in unnecessary cognitive overhead for the reader who has to "think about it" for it to make sense.

"SILCFGEdge" is also not a very attractive name because of the two abbreviations. If you had a nice alternative to "CFGEdge" that was less biased to the beginning/end (like Successor/Predecessor are), I probably wouldn't object.

Ok. Maybe SILControlFlowEdge?

I can see your point here, but a big renaming change like this is disruptive. With all the other things we’re trying to get done now, is this really the right time to pay the cost of that?


(John McCall) #7

Hey everyone.

I am currently doing some small fixes to SILSuccessor (adding some comments and fixing some issues exposed by LLVM upstream). As I read the code it became pretty apparent that the name is a misnomer... SILSuccessor is not just representing a successor, rather it is representing a whole CFG edge. This can be seen in how SILSuccessor is used to iterate over the predecessors of the block.

With that in mind, I would like to rename SILSuccessor to SILCFGEdge. It will make it much clearer without knowing any context what this data structure is used for.

Any objections, disagreements, flames, etc?

It seems a little unnecessary to me. The successor relationship is an edge, and all the edges of the local CFG are successor relationships. I guess it looks a little funny that the edges into a block are represented by "successors", but I think when you think about it it makes sense.

IMO this is more of an issue than something "looking funny". Using code named "successor" to look up the "predecessors" of a block is misleading and results in unnecessary cognitive overhead for the reader who has to "think about it" for it to make sense.

Uh, sure, but this is also not something most people have to deal with a ton, and it's a learn-once-and-remember sort of thing.

"SILCFGEdge" is also not a very attractive name because of the two abbreviations. If you had a nice alternative to "CFGEdge" that was less biased to the beginning/end (like Successor/Predecessor are), I probably wouldn't object.

Ok. Maybe SILControlFlowEdge?

A bit elaborate, but it could work. Honestly, this is not a type I have to write out much.

John.

···

On Apr 26, 2017, at 6:11 PM, Michael Gottesman <mgottesman@apple.com> wrote:

On Apr 26, 2017, at 1:44 PM, John McCall <rjmccall@apple.com> wrote:

On Apr 26, 2017, at 4:24 PM, Michael Gottesman via swift-dev <swift-dev@swift.org> wrote:


(Bob Wilson) #8

How about just SILEdge?

(Honestly I’d prefer to keep it as SILSuccessor and avoid the churn from this, but if it is important to you, I won’t object to changing it.)

···

On Apr 26, 2017, at 6:43 PM, John McCall via swift-dev <swift-dev@swift.org> wrote:

On Apr 26, 2017, at 6:11 PM, Michael Gottesman <mgottesman@apple.com> wrote:

On Apr 26, 2017, at 1:44 PM, John McCall <rjmccall@apple.com> wrote:

On Apr 26, 2017, at 4:24 PM, Michael Gottesman via swift-dev <swift-dev@swift.org> wrote:
Hey everyone.

I am currently doing some small fixes to SILSuccessor (adding some comments and fixing some issues exposed by LLVM upstream). As I read the code it became pretty apparent that the name is a misnomer... SILSuccessor is not just representing a successor, rather it is representing a whole CFG edge. This can be seen in how SILSuccessor is used to iterate over the predecessors of the block.

With that in mind, I would like to rename SILSuccessor to SILCFGEdge. It will make it much clearer without knowing any context what this data structure is used for.

Any objections, disagreements, flames, etc?

It seems a little unnecessary to me. The successor relationship is an edge, and all the edges of the local CFG are successor relationships. I guess it looks a little funny that the edges into a block are represented by "successors", but I think when you think about it it makes sense.

IMO this is more of an issue than something "looking funny". Using code named "successor" to look up the "predecessors" of a block is misleading and results in unnecessary cognitive overhead for the reader who has to "think about it" for it to make sense.

Uh, sure, but this is also not something most people have to deal with a ton, and it's a learn-once-and-remember sort of thing.

"SILCFGEdge" is also not a very attractive name because of the two abbreviations. If you had a nice alternative to "CFGEdge" that was less biased to the beginning/end (like Successor/Predecessor are), I probably wouldn't object.

Ok. Maybe SILControlFlowEdge?

A bit elaborate, but it could work. Honestly, this is not a type I have to write out much.


(John McCall) #9

Hey everyone.

I am currently doing some small fixes to SILSuccessor (adding some comments and fixing some issues exposed by LLVM upstream). As I read the code it became pretty apparent that the name is a misnomer... SILSuccessor is not just representing a successor, rather it is representing a whole CFG edge. This can be seen in how SILSuccessor is used to iterate over the predecessors of the block.

With that in mind, I would like to rename SILSuccessor to SILCFGEdge. It will make it much clearer without knowing any context what this data structure is used for.

Any objections, disagreements, flames, etc?

It seems a little unnecessary to me. The successor relationship is an edge, and all the edges of the local CFG are successor relationships. I guess it looks a little funny that the edges into a block are represented by "successors", but I think when you think about it it makes sense.

IMO this is more of an issue than something "looking funny". Using code named "successor" to look up the "predecessors" of a block is misleading and results in unnecessary cognitive overhead for the reader who has to "think about it" for it to make sense.

Uh, sure, but this is also not something most people have to deal with a ton, and it's a learn-once-and-remember sort of thing.

"SILCFGEdge" is also not a very attractive name because of the two abbreviations. If you had a nice alternative to "CFGEdge" that was less biased to the beginning/end (like Successor/Predecessor are), I probably wouldn't object.

Ok. Maybe SILControlFlowEdge?

A bit elaborate, but it could work. Honestly, this is not a type I have to write out much.

How about just SILEdge?

SILEdge seems a little vague. Also less obvious to someone who isn't thinking of the CFG as primarily a graph rather than a control-flow relationship. But it could work.

I agree that I also don't mind just sticking with SILSuccessor. In general, it would be good to see positive support from more than one person for this rename before agreeing to it.

John.

···

On Apr 27, 2017, at 12:01 AM, Bob Wilson <bob.wilson@apple.com> wrote:

On Apr 26, 2017, at 6:43 PM, John McCall via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

On Apr 26, 2017, at 6:11 PM, Michael Gottesman <mgottesman@apple.com <mailto:mgottesman@apple.com>> wrote:

On Apr 26, 2017, at 1:44 PM, John McCall <rjmccall@apple.com <mailto:rjmccall@apple.com>> wrote:

On Apr 26, 2017, at 4:24 PM, Michael Gottesman via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

(Honestly I’d prefer to keep it as SILSuccessor and avoid the churn from this, but if it is important to you, I won’t object to changing it.)