Rubberduck.parsing c#6&7 update #3582
Conversation
| { | ||
| //Note - Enums and Types should enumerate *last*. That will prevent a duplicate module in the unlikely(?) | ||
| //instance where the TypeLib defines a module named "Enums" or "Types". | ||
| return _modules.Cast<IComType>() |
There was a problem hiding this comment.
Why does this comment get erased?
There was a problem hiding this comment.
Learning mistake. For accessors like this with notes is it preferred to leave in { ... get { ... } } format?
| @@ -29,8 +27,8 @@ public override IValue Evaluate() | |||
| var constantValue = _expression.Evaluate(); | |||
| _symbolTable.Add(identifier, constantValue); | |||
| return new LivelinessExpression( | |||
| isAlive: new ConstantExpression(new BoolValue(false)), | |||
| tokens: _tokens).Evaluate(); | |||
There was a problem hiding this comment.
Removing unnecessary argument names is not always good. They are usually there to aid readability.
There was a problem hiding this comment.
Will revert. Apologies for any overeager changes.
| _stream = stream; | ||
| _tokenStream = tokenStream; | ||
| _symbolTable = symbolTable; | ||
| _stream = stream ?? throw new ArgumentNullException(nameof(stream)); |
There was a problem hiding this comment.
As I have already stated elsewhere, I do not really like guard clauses to be hidden away like this.
| @@ -239,19 +232,20 @@ private void AddSubtype(Declaration subtype) | |||
| private void RemoveSubtype(Declaration subtype) | |||
| { | |||
| InvalidateCachedIsGlobal(); | |||
| byte dummy; | |||
| _subtypes.TryRemove(subtype, out dummy); | |||
| _subtypes.TryRemove(subtype, out var dummy); | |||
| { | ||
| () => | ||
| _unresolved = unresolvedMemberDeclarations | ||
| .ToList(), |
There was a problem hiding this comment.
If you do such a change, please also remove the now clunky line break.
| } | ||
|
|
||
| string token; | ||
| return HasTypeHint(out token); |
There was a problem hiding this comment.
Doesn't removing this eliminate the caching?
There was a problem hiding this comment.
If we want to take advantage of caching, the correct solution here is to introduce a Lazy instead
| { | ||
| foreach (var positionalArgument in argumentList.argument()) | ||
| continue; |
There was a problem hiding this comment.
Do we really want to invert such conditions that lead to a continue or break statement?
There was a problem hiding this comment.
#itdepends, I guess. in this case the benefit seems marginal
| @@ -48,11 +47,13 @@ public override void EnterSubStmt(VBAParser.SubStmtContext context) | |||
|
|
|||
| public override void ExitSubStmt(VBAParser.SubStmtContext context) | |||
| { | |||
| if(_currentScopeAttributes.Any()) | |||
| if (!_currentScopeAttributes.Any()) | |||
There was a problem hiding this comment.
Why invert this condition?
There was a problem hiding this comment.
Over-eagerness with a new tool.
| @@ -64,11 +65,13 @@ public override void EnterFunctionStmt(VBAParser.FunctionStmtContext context) | |||
|
|
|||
| public override void ExitFunctionStmt(VBAParser.FunctionStmtContext context) | |||
| { | |||
| if(_currentScopeAttributes.Any()) | |||
| if (!_currentScopeAttributes.Any()) | |||
| @@ -80,11 +83,13 @@ public override void EnterPropertyGetStmt(VBAParser.PropertyGetStmtContext conte | |||
|
|
|||
| public override void ExitPropertyGetStmt(VBAParser.PropertyGetStmtContext context) | |||
| { | |||
| if(_currentScopeAttributes.Any()) | |||
| if (!_currentScopeAttributes.Any()) | |||
| @@ -96,11 +101,13 @@ public override void EnterPropertyLetStmt(VBAParser.PropertyLetStmtContext conte | |||
|
|
|||
| public override void ExitPropertyLetStmt(VBAParser.PropertyLetStmtContext context) | |||
| { | |||
| if(_currentScopeAttributes.Any()) | |||
| if (!_currentScopeAttributes.Any()) | |||
| @@ -112,11 +119,13 @@ public override void EnterPropertySetStmt(VBAParser.PropertySetStmtContext conte | |||
|
|
|||
| public override void ExitPropertySetStmt(VBAParser.PropertySetStmtContext context) | |||
| { | |||
| if(_currentScopeAttributes.Any()) | |||
| if (!_currentScopeAttributes.Any()) | |||
| { | ||
| code = File.ReadAllText(path, Encoding.Default); //The VBE exports encoded in the current ANSI codepage from the windows settings. | ||
| } | ||
| var code = File.ReadAllText(path, module.ComponentType == ComponentType.Document ? Encoding.UTF8 : Encoding.Default); |
There was a problem hiding this comment.
Why remove the comments? They are rather helpful here.
|
In contrast to what the title says, this PR does not merrily perform C#6 and C#7 syntactic updates. There are also some helpful comments that got erased in the refactorings. |
retailcoder
added
the
technical-debt
This reverts commit 083f11f.
…nto Rubberduck.Parsing_C#6&7_Update
…nto Rubberduck.Parsing_C#6&7_Update
|
This PR touches on what's essentially the very core of RD; implementing the requested changes and keeping it up-to-date is becoming increasingly important. |
Vogel612
dismissed
their stale review
review is largely addressed. Remaining open points are not blocking
|
I'll look at syncing this with the current build and getting it finalized by weekends end. |
Codecov Report
@@ Coverage Diff @@
## next #3582 +/- ##
==========================================
- Coverage 71.21% 71.19% -0.03%
==========================================
Files 1162 1161 -1
Lines 80274 80169 -105
==========================================
- Hits 57167 57076 -91
+ Misses 23107 23093 -14
Continue to review full report at Codecov.
|
| @@ -81,28 +81,31 @@ public bool HasExplicitCallStatement() | |||
| return Context.Parent is VBAParser.CallStmtContext && ((VBAParser.CallStmtContext)Context).CALL() != null; | |||
| } | |||
|
|
|||
| private Lazy<bool> _hasTypeHint = new Lazy<bool>(() => ComputeHasTypeHint()); | |||
There was a problem hiding this comment.
I'm pretty sure you don't need the line 84-91. You only need one line here -- private bool HasTypeHint() => new Lazy<bool>(() => ComputeTypeHint()).Value; (untested aircode!)
There was a problem hiding this comment.
not correct. that's not caching the value again. What's necessary is initializing _hasTypeHint in the ctor. @bclothier your code instantiates a new Lazy every invocation of HasTypeHint(), which is exactly what we wanted to avoid.
HasTypeHint should become: public bool HasTypeHint() => _hasTypeHint.Value;
…nto Rubberduck.Parsing_C#6&7_Update
| @@ -82,15 +84,7 @@ public bool HasExplicitCallStatement() | |||
| } | |||
|
|
|||
| private Lazy<bool> _hasTypeHint; | |||
There was a problem hiding this comment.
Given that this is only initialized in the ctor, it can be made readonly or converted into an expression-bodied statement.
retailcoder
added
PR-Status: Conflicting
F5 and Unit tested.