Hello,
More than once, I’ve made typos while hacking on the constraint system because the key variables are often named “type1” and “type2”. Beyond sharing 4 out of 5 characters, the ‘1’ and the ‘2’ are right next to each other on the keyboard, and typing the other by accident results in code that still compiles.
Are “type1” and “type2” really the most apt names? Is something preventing better names from being used? Wouldn’t “fromTy” and “toTy” be better and less typo prone?
Thanks,
Dave
It’s an unfortunate naming scheme, but I would hesitate to use notions like “from” and “to”, especially in CSSimplify where different matchers are handed types in whatever order variance requires. More critical here are the scoping issues. There are a bunch of places, especially in matchTypes itself, where type1 and type2 are re-assigned in branches during the matching process. It makes it more of a pain to debug than it should be.
~Robert Widmann
On Aug 17, 2017, at 9:55 AM, David Zarzycki via swift-dev <swift-dev@swift.org> wrote:
Hello,
More than once, I’ve made typos while hacking on the constraint system because the key variables are often named “type1” and “type2”. Beyond sharing 4 out of 5 characters, the ‘1’ and the ‘2’ are right next to each other on the keyboard, and typing the other by accident results in code that still compiles.
Are “type1” and “type2” really the most apt names? Is something preventing better names from being used? Wouldn’t “fromTy” and “toTy” be better and less typo prone?
Thanks,
Dave
_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev
It’s an unfortunate naming scheme, but I would hesitate to use notions like “from” and “to”, especially in CSSimplify where different matchers are handed types in whatever order variance requires.
Thanks Robert,
That is what I feared might exist. Can you give an example or two where “from” and “to” would be imprecise or wrong?
More critical here are the scoping issues. There are a bunch of places, especially in matchTypes itself, where type1 and type2 are re-assigned in branches during the matching process. It makes it more of a pain to debug than it should be.
I just took a look at matchTypes(), where my last typo was, and it looks like the reassignment is largely done out of convenience, not necessity. Is this worth fixing? Most, if not all of them look easy to fix via some judicious use of ‘const’ and a few new local variables (instead of reassignment). It would make reasoning about the code easier for both newbies and experts alike.
Dave
On Aug 17, 2017, at 13:03, Robert Widmann <rwidmann@apple.com> wrote:
~Robert Widmann
On Aug 17, 2017, at 9:55 AM, David Zarzycki via swift-dev <swift-dev@swift.org> wrote:
Hello,
More than once, I’ve made typos while hacking on the constraint system because the key variables are often named “type1” and “type2”. Beyond sharing 4 out of 5 characters, the ‘1’ and the ‘2’ are right next to each other on the keyboard, and typing the other by accident results in code that still compiles.
Are “type1” and “type2” really the most apt names? Is something preventing better names from being used? Wouldn’t “fromTy” and “toTy” be better and less typo prone?
Thanks,
Dave
_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev
It’s an unfortunate naming scheme, but I would hesitate to use notions like “from” and “to”, especially in CSSimplify where different matchers are handed types in whatever order variance requires.
Thanks Robert,
That is what I feared might exist. Can you give an example or two where “from” and “to” would be imprecise or wrong?
More critical here are the scoping issues. There are a bunch of places, especially in matchTypes itself, where type1 and type2 are re-assigned in branches during the matching process. It makes it more of a pain to debug than it should be.
I just took a look at matchTypes(), where my last typo was, and it looks like the reassignment is largely done out of convenience, not necessity. Is this worth fixing? Most, if not all of them look easy to fix via some judicious use of ‘const’ and a few new local variables (instead of reassignment). It would make reasoning about the code easier for both newbies and experts alike.
I’m hoping to devote some time to some refactoring throughout CSSimplify with the hope of simplifying (ha!) the code and making it more accessible and easier to maintain. Eliminating reassignments would be part of that.
It would be great to see other people help make some of those improvements as well, but I would rather see renaming done as part of other clean-up and refactoring as opposed to mass renames on their own. Having said that I think it’s totally reasonable to have small commits that e.g. do small-scale renaming within a single function per commit. The specific matchTypes() function itself is a bit of a challenge in that it’s ~1000 lines of code and really needs to be simplified and split up, which I think will aid in producing fewer issues with reassignment and fewer similar-but-slightly-different names.
In general I would like to see names that are more visually distinct, but there aren’t always clearly great names. One convention used in some of the code is to use ‘first’ and ‘second’ which correspond to the first and second types in the constraint that the types are coming from. Despite these names lacking any real specificity, they are very visually distinct.
Mark
On Aug 17, 2017, at 10:28 AM, David Zarzycki via swift-dev <swift-dev@swift.org> wrote:
On Aug 17, 2017, at 13:03, Robert Widmann <rwidmann@apple.com> wrote:
Dave
~Robert Widmann
On Aug 17, 2017, at 9:55 AM, David Zarzycki via swift-dev <swift-dev@swift.org> wrote:
Hello,
More than once, I’ve made typos while hacking on the constraint system because the key variables are often named “type1” and “type2”. Beyond sharing 4 out of 5 characters, the ‘1’ and the ‘2’ are right next to each other on the keyboard, and typing the other by accident results in code that still compiles.
Are “type1” and “type2” really the most apt names? Is something preventing better names from being used? Wouldn’t “fromTy” and “toTy” be better and less typo prone?
Thanks,
Dave
_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev
It’s an unfortunate naming scheme, but I would hesitate to use notions like “from” and “to”, especially in CSSimplify where different matchers are handed types in whatever order variance requires.
Thanks Robert,
That is what I feared might exist. Can you give an example or two where “from” and “to” would be imprecise or wrong?
More critical here are the scoping issues. There are a bunch of places, especially in matchTypes itself, where type1 and type2 are re-assigned in branches during the matching process. It makes it more of a pain to debug than it should be.
I just took a look at matchTypes(), where my last typo was, and it looks like the reassignment is largely done out of convenience, not necessity. Is this worth fixing? Most, if not all of them look easy to fix via some judicious use of ‘const’ and a few new local variables (instead of reassignment). It would make reasoning about the code easier for both newbies and experts alike.
I’m hoping to devote some time to some refactoring throughout CSSimplify with the hope of simplifying (ha!) the code and making it more accessible and easier to maintain. Eliminating reassignments would be part of that.
It would be great to see other people help make some of those improvements as well, but I would rather see renaming done as part of other clean-up and refactoring as opposed to mass renames on their own. Having said that I think it’s totally reasonable to have small commits that e.g. do small-scale renaming within a single function per commit. The specific matchTypes() function itself is a bit of a challenge in that it’s ~1000 lines of code and really needs to be simplified and split up, which I think will aid in producing fewer issues with reassignment and fewer similar-but-slightly-different names.
Hi Mark,
Interesting that you should mention breaking up matchTypes() and potentially others. What if any convention does Swift have around when functions becomes “too” long or “too” indented? I ask because I’m sometimes tempted to break large functions apart *just to understand* their code flow / theory of operation.
I’ve also been tempted to refactor parts of the compiler to use switch statements more often. (Again, just to make the code easier to reason about.) What if any thought do you have about the trade off between a series of ‘if’ statements and using ’switch’? For example, in matchTypes(), I’ve been tempted to change the branch heavy code after the parallel structure decomposition to use switch statements as a learning exercise. Is that a reasonable change, or is there something intrinsically complicated about matchTypes() that requires such branch heavy logic?
In general I would like to see names that are more visually distinct, but there aren’t always clearly great names. One convention used in some of the code is to use ‘first’ and ‘second’ which correspond to the first and second types in the constraint that the types are coming from. Despite these names lacking any real specificity, they are very visually distinct.
Ya, first and second would certainly be better. There is also no harm in aliasing the variables in nested scopes to add clarify and avoid bugs. For example,
if (kind == subtype) {
// Rename the constraint variables to ease reasoning
auto fromTy = first;
auto toTy = second;
…
}
Cheers,
Dave
On Aug 17, 2017, at 15:28, Mark Lacey <mark.lacey@apple.com> wrote:
On Aug 17, 2017, at 10:28 AM, David Zarzycki via swift-dev <swift-dev@swift.org> wrote:
On Aug 17, 2017, at 13:03, Robert Widmann <rwidmann@apple.com> wrote:
It’s an unfortunate naming scheme, but I would hesitate to use notions like “from” and “to”, especially in CSSimplify where different matchers are handed types in whatever order variance requires.
Thanks Robert,
That is what I feared might exist. Can you give an example or two where “from” and “to” would be imprecise or wrong?
More critical here are the scoping issues. There are a bunch of places, especially in matchTypes itself, where type1 and type2 are re-assigned in branches during the matching process. It makes it more of a pain to debug than it should be.
I just took a look at matchTypes(), where my last typo was, and it looks like the reassignment is largely done out of convenience, not necessity. Is this worth fixing? Most, if not all of them look easy to fix via some judicious use of ‘const’ and a few new local variables (instead of reassignment). It would make reasoning about the code easier for both newbies and experts alike.
I’m hoping to devote some time to some refactoring throughout CSSimplify with the hope of simplifying (ha!) the code and making it more accessible and easier to maintain. Eliminating reassignments would be part of that.
It would be great to see other people help make some of those improvements as well, but I would rather see renaming done as part of other clean-up and refactoring as opposed to mass renames on their own. Having said that I think it’s totally reasonable to have small commits that e.g. do small-scale renaming within a single function per commit. The specific matchTypes() function itself is a bit of a challenge in that it’s ~1000 lines of code and really needs to be simplified and split up, which I think will aid in producing fewer issues with reassignment and fewer similar-but-slightly-different names.
Hi Mark,
Interesting that you should mention breaking up matchTypes() and potentially others. What if any convention does Swift have around when functions becomes “too” long or “too” indented? I ask because I’m sometimes tempted to break large functions apart *just to understand* their code flow / theory of operation.
I don’t think we really have any written conventions regarding how long is too long or how indented is too indented. Personally I tend to favor short simple functions with minimal control flow. If a piece of code in and of itself has a well defined purpose, it generally makes sense for it to be a separate function. One of the practical issues people run into here is with naming. Sometimes despite having a clear purpose it can be challenging to come up with a reasonable name, and I think that alone sometimes results in enough friction that existing functions just grow larger and larger.
I absolutely think if you’re having a difficult time understanding a large function due to the length, amount of control flow, etc., it absolutely makes sense to split it up. If you’re doing it for your own understanding then you’re probably not the only one who is going to run into difficulty with understanding that code.
I’ve also been tempted to refactor parts of the compiler to use switch statements more often. (Again, just to make the code easier to reason about.) What if any thought do you have about the trade off between a series of ‘if’ statements and using ’switch’? For example, in matchTypes(), I’ve been tempted to change the branch heavy code after the parallel structure decomposition to use switch statements as a learning exercise. Is that a reasonable change, or is there something intrinsically complicated about matchTypes() that requires such branch heavy logic?
I think the way I’d answer those questions is trying and seeing what the outcome is like. I don’t know that there are always great general guidelines.
For matchTypes() specifically, one thought I’ve had in the past but never actually tried is to try to split out the code handling three distinct cases:
- Both types are type variables
- Both types are concrete
- One type is a type variable
The first case involves a relatively small amount of code.
The last two cases are covered by a fair bit of shared code, but also a fair bit of distinct code (there are a couple hundred lines of code under a 'typeVar1 || typeVar2’ test, and quite a bit of code of the form ‘if (concrete && …)’.
In general I would like to see names that are more visually distinct, but there aren’t always clearly great names. One convention used in some of the code is to use ‘first’ and ‘second’ which correspond to the first and second types in the constraint that the types are coming from. Despite these names lacking any real specificity, they are very visually distinct.
Ya, first and second would certainly be better. There is also no harm in aliasing the variables in nested scopes to add clarify and avoid bugs. For example,
if (kind == subtype) {
// Rename the constraint variables to ease reasoning
auto fromTy = first;
auto toTy = second;
…
}
Sure, that kind of aliasing totally makes sense if it aids in comprehension. I suspect in many cases, though, it might make even more sense to split out a separate function with parameters named fromTy and toTo.
Mark
On Aug 18, 2017, at 5:35 AM, David Zarzycki <zarzycki@icloud.com> wrote:
On Aug 17, 2017, at 15:28, Mark Lacey <mark.lacey@apple.com <mailto:mark.lacey@apple.com>> wrote:
On Aug 17, 2017, at 10:28 AM, David Zarzycki via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:
On Aug 17, 2017, at 13:03, Robert Widmann <rwidmann@apple.com <mailto:rwidmann@apple.com>> wrote: