Fix range operator #5732
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -785,6 +785,92 @@ internal static object JoinOperator(ExecutionContext context, IScriptExtent erro | |
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// The implementation of the PowerShell range operator. | ||
| /// </summary> | ||
| /// <param name="lval">The object on which to start.</param> | ||
| /// <param name="rval">The object on which to stop.</param> | ||
| /// <returns>The array of objects.</returns> | ||
| internal static object RangeOperator(object lval, object rval) | ||
| { | ||
| var lbase = PSObject.Base(lval); | ||
| var rbase = PSObject.Base(rval); | ||
|
|
||
| // If both arguments is [char] type or [string] type with length==1 | ||
| // return objects of [char] type. | ||
| // In special case "0".."9" return objects of [int] type. | ||
| if (AsChar(lbase) is char lc && AsChar(rbase) is char rc) | ||
| { | ||
| return CharOps.Range(lc, rc); | ||
| } | ||
|
|
||
| // As a last resort, the range operator tries to return objects of [int] type. | ||
| // 1..10 | ||
| // "1".."10" | ||
| // [int]"1"..[int]"10" | ||
| var l = Convert.ToInt32(lbase); | ||
| var r = Convert.ToInt32(rbase); | ||
|
|
||
| return IntOps.Range(l, r); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// The implementation of an enumerator for the PowerShell range operator. | ||
| /// </summary> | ||
| /// <param name="lval">The object on which to start.</param> | ||
| /// <param name="rval">The object on which to stop.</param> | ||
| /// <returns>The enumerator.</returns> | ||
| internal static IEnumerator GetRangeEnumerator(object lval, object rval) | ||
| { | ||
| var lbase = PSObject.Base(lval); | ||
| var rbase = PSObject.Base(rval); | ||
|
|
||
| // If both arguments is [char] type or [string] type with length==1 | ||
| // return objects of [char] type. | ||
| // In special case "0".."9" return objects of [int] type. | ||
| if (AsChar(lbase) is char lc && AsChar(rbase) is char rc) | ||
| { | ||
| return new CharRangeEnumerator(lc, rc); | ||
| } | ||
|
|
||
| // As a last resort, the range operator tries to return objects of [int] type. | ||
| // 1..10 | ||
| // "1".."10" | ||
| // [int]"1"..[int]"10" | ||
| var l = Convert.ToInt32(lbase); | ||
| var r = Convert.ToInt32(rbase); | ||
|
|
||
| return new RangeEnumerator(l, r); | ||
| } | ||
|
|
||
| // Help function for Range operator. | ||
| // | ||
| // In common case: | ||
| // for [char] type | ||
| // for [string] type and length == 1 | ||
| // return objects of [char] type: | ||
| // [char]'A'..[char]'Z' | ||
| // [char]'A'..'Z' | ||
| // [char]'A'.."Z" | ||
| // 'A'..[char]'Z' | ||
| // "A"..[char]'Z' | ||
| // "A".."Z" | ||
| // [char]"A"..[string]"Z" | ||
| // "A"..[char]"Z" | ||
| // [string]"A".."Z" | ||
| // and so on. | ||
| // | ||
| // In special case: | ||
| // "0".."9" | ||
| // return objects of [int] type. | ||
| private static object AsChar(object obj) | ||
| { | ||
| if (obj is char) return obj; | ||
| if (obj is string str && str.Length == 1 && !Char.IsDigit(str, 0)) return str[0]; | ||
| return null; | ||
| } | ||
|
|
||
|
|
||
| /// <summary> | ||
| /// The implementation of the PowerShell -replace operator.... | ||
| /// </summary> | ||
|
|
@@ -1478,7 +1564,13 @@ internal int UpperBound | |
| } | ||
|
|
||
| private int _current; | ||
|
|
||
| Object IEnumerator.Current | ||
|
Contributor
There was a problem hiding this comment. Can you explain why this method was added? It seems like we only need the explicit method impl or the other, but not both.
Collaborator
Author
There was a problem hiding this comment. I saw it in CoreFX implementations ( sample ). I don't remember exactly - it seems I'm checking that it doesn't work at all without this method if we use enheritance and call by IEnumerator interface.
Contributor
There was a problem hiding this comment. The example you point to is needed to implement the strongly typed interface |
||
| { | ||
| get { return Current; } | ||
| } | ||
|
|
||
| public virtual int Current | ||
| { | ||
| get { return _current; } | ||
| } | ||
|
|
@@ -1521,6 +1613,70 @@ public bool MoveNext() | |
| return true; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// The simple enumerator class is used for the range operator '..' | ||
| /// in expressions like 'A'..'B' | ForEach-Object { $_ } | ||
| /// </summary> | ||
| internal class CharRangeEnumerator : IEnumerator | ||
|
Contributor
There was a problem hiding this comment. I'm curious if we should start following the one-type-per-file rule https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1402.md
Contributor
There was a problem hiding this comment. It is often useful to have related types in the same file, so I'm strongly opposed to following this rule. I'm strongly in favor of organizing code in ways that make sense to the author and reviewers. We shouldn't blindly follow the guidelines of some tool.
Contributor
There was a problem hiding this comment. I agree that it make sense to group related code and that following the rules blindly is silly. But, some of the files in this project are so massive that there is 0 benefit gained by the code being grouped, IMO. If I need to have 3 tabs open on the same file to work on it effectively, the code is probably better split, IMO. Doesn't have to be strictly followed, but, maybe give some serious consideration?
Contributor
There was a problem hiding this comment. Are you suggesting the author of said code saw 0 benefit of a huge file, because you might be saying that to said author. (I'm the primary author of 4 of the 10 largest non-generated files in the repo.) To pick on the largest file - Ast.cs - there are at least 72 classes in that file. When I know I'm looking for something Ast related, but not sure what, if I end up with 72 open tabs that I need to close, I'm less productive than if I open 1 file. And if you needed 3 tabs open to effectively work on some code, you needed 3 tabs to see some code, 1 file or 3 files, right? I do realize There is always room for improvement, but it should be thoughtful and not rely on some tool - tools are good at enforcing things that are easy to check, less good at providing good judgement.
Contributor
There was a problem hiding this comment.
Not at all. I'm suggesting I see 0 benefit.
I know 🙂 .
Right, I'm saying I see no benefit to these being in the same file, where I do see benefit in it being in separate files. I think it's a difference of preference, I prefer smaller files grouped in folders over monolith files. It's easier to jump between code in different files using the IDE, IMO. When the code is in the same file, the default action is to jump to the code in the current tab. When it is in another file, a new tab opens with that file and my current place in the previous file is preserved. *shrugs I'm also trying to approach this from a maintainable open source project perspective. If this rule doen't makes sense for this project, then it doesn't make sense and we don't use it. I just want to have something in place that can help maintain and encourage an enforceable style for the life of the project, preferably through CI. It can help shift the burden from reviewers to contributors. If I'm wasting my efforts there, then I will gladly redirect my attention elsewhere.
Contributor
There was a problem hiding this comment. Should this implement
Collaborator
Author
There was a problem hiding this comment. I think we cast all to
Contributor
There was a problem hiding this comment. The code is basically the same as RangeEnumerator, I wonder if there is a clever way we could avoid the duplication. One thought - you could maybe derive from RangeEnumerator and just override Current. It's probably not worth spending time on this though - C# doesn't handle this scenario nicely like C++ template would.
Collaborator
Author
There was a problem hiding this comment. I started with this but it looked too confusing. In addition, I thought that if we wanted to enhance this operator in the future, it would be easier if each type would have its own enumerator. Also possible we can make optimization and reduce string allocations. |
||
| { | ||
| private int _increment = 1; | ||
|
|
||
| private bool _firstElement = true; | ||
|
|
||
| public CharRangeEnumerator(char lowerBound, char upperBound) | ||
|
Contributor
There was a problem hiding this comment. This should probably go with the other methods in the class.
Collaborator
Author
There was a problem hiding this comment. I use your link https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1201.md
Contributor
There was a problem hiding this comment. Ah I missed that this was the constructor... My bad |
||
| { | ||
| LowerBound = lowerBound; | ||
| Current = lowerBound; | ||
| UpperBound = upperBound; | ||
| if (lowerBound > upperBound) | ||
| _increment = -1; | ||
| } | ||
|
|
||
| Object IEnumerator.Current | ||
| { | ||
| get { return Current; } | ||
| } | ||
|
|
||
| internal char LowerBound | ||
| { | ||
| get; private set; | ||
| } | ||
|
|
||
| internal char UpperBound | ||
| { | ||
| get; private set; | ||
| } | ||
|
|
||
| public char Current | ||
| { | ||
| get; private set; | ||
| } | ||
|
|
||
| public bool MoveNext() | ||
| { | ||
| if (_firstElement) | ||
| { | ||
| _firstElement = false; | ||
| return true; | ||
| } | ||
|
|
||
| if (Current == UpperBound) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| Current = (char)(Current + _increment); | ||
| return true; | ||
| } | ||
|
|
||
| public void Reset() | ||
| { | ||
| Current = LowerBound; | ||
| _firstElement = true; | ||
| } | ||
| } | ||
|
|
||
| #endregion RangeEnumerator | ||
|
|
||
| #region InterpreterError | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should swap places with
private int _current;so that the field can be closed to the property it backs.