-
Notifications
You must be signed in to change notification settings - Fork 13
Incremental and porting updates #110
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Text; | ||
| using Codelyzer.Analysis.Model; | ||
|
|
||
| namespace CTA.Rules.Models | ||
| { | ||
| public class IDEFileActions | ||
| { | ||
| public string FilePath { get; set; } | ||
| public TextSpan TextSpan { get; set; } | ||
| public string Description { get; set; } | ||
| public IList<TextChange> TextChanges { get; set; } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| using System; | ||
| using Microsoft.CodeAnalysis; | ||
|
|
||
| namespace CTA.Rules.Models | ||
| { | ||
| public class TextChange | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A doc comment for this class would be helpful as well |
||
| { | ||
| public string NewText { get; set; } | ||
| public FileLinePositionSpan FileLinePositionSpan { get; set; } | ||
|
|
||
| public bool Equals(TextChange textChange) | ||
| { | ||
| if (textChange == null) return false; | ||
|
|
||
| return NewText == textChange.NewText | ||
| && FileLinePositionSpan.Equals(textChange.FileLinePositionSpan); | ||
| } | ||
|
|
||
| public override bool Equals(object obj) | ||
| { | ||
| return Equals(obj as TextChange); | ||
| } | ||
|
|
||
| public override int GetHashCode() | ||
| { | ||
| return HashCode.Combine(FileLinePositionSpan, NewText); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| using System.Collections.Generic; | ||
| using Codelyzer.Analysis.Model; | ||
| using System.Collections.Concurrent; | ||
| using System.Collections.Generic; | ||
| using CTA.Rules.Config; | ||
| using TextSpan = Codelyzer.Analysis.Model.TextSpan; | ||
|
|
||
| namespace CTA.Rules.Models.Tokens | ||
| { | ||
|
|
@@ -24,6 +25,7 @@ public NodeToken() | |
| ProjectLevelActions = new List<ProjectLevelAction>(); | ||
| ProjectFileActions = new List<ProjectLevelAction>(); | ||
| TargetCPU = new List<string>(); | ||
| TextChanges = new List<TextChange>(); | ||
| } | ||
| public string Key { get; set; } | ||
| public string TrimmedKey => !string.IsNullOrEmpty(Key) ? Utils.EscapeAllWhitespace(Key) : string.Empty; | ||
|
|
@@ -32,6 +34,7 @@ public NodeToken() | |
| public string FullKey { get; set; } | ||
| public TextSpan TextSpan { get; set; } | ||
| public string Description { get; set; } | ||
| public IList<TextChange> TextChanges { get; set; } | ||
| public List<string> TargetCPU { get; set; } | ||
| public List<AttributeAction> AttributeActions { get; set; } | ||
| public List<AttributeAction> AttributeListActions { get; set; } | ||
|
|
@@ -51,5 +54,27 @@ public NodeToken() | |
|
|
||
| public NodeToken Clone() => (NodeToken)this.MemberwiseClone(); | ||
|
|
||
| public List<GenericAction> AllActions | ||
| { | ||
| get | ||
| { | ||
| var allActions = new List<GenericAction>(); | ||
| allActions.AddRange(AttributeActions); | ||
| allActions.AddRange(AttributeListActions); | ||
| allActions.AddRange(MethodDeclarationActions); | ||
| allActions.AddRange(ClassDeclarationActions); | ||
| allActions.AddRange(InterfaceDeclarationActions); | ||
| allActions.AddRange(ElementAccessActions); | ||
| allActions.AddRange(MemberAccessActions); | ||
| allActions.AddRange(IdentifierNameActions); | ||
| allActions.AddRange(InvocationExpressionActions); | ||
| allActions.AddRange(MemberAccessActions); | ||
| allActions.AddRange(UsingActions); | ||
| allActions.AddRange(ObjectCreationExpressionActions); | ||
| allActions.AddRange(NamespaceActions); | ||
| return allActions; | ||
| } | ||
| } | ||
|
Comment on lines
+57
to
+77
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a specific reason or requirement for creating a new List when this property is called? If not, convert If this behavior is intentional, it may be more appropriate to convert this property to a method as properties are used to retrieve state rather than generate new objects.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand. How would this cause a memory leak?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Memory leak might not be the best term here, but my thought is that it's possible to inadvertently consume more resources than expected. See the below example for what I was thinking, but if this is a nonconcern, we can forgo this modification. Trivial example: var allActions1 = nodeToken.AllActions;
var allActions2 = nodeToken.AllActions;With the current implementation, this would allocate 2x the memory than the suggested implementation: private List<GenericAction> _allActions;
public List<GenericAction> AllActions
{
get
{
_allActions = new List<GenericAction>();
_allActions.AddRange(AttributeActions);
_allActions.AddRange(AttributeListActions);
_allActions.AddRange(MethodDeclarationActions);
_allActions.AddRange(ClassDeclarationActions);
_allActions.AddRange(InterfaceDeclarationActions);
_allActions.AddRange(ElementAccessActions);
_allActions.AddRange(MemberAccessActions);
_allActions.AddRange(IdentifierNameActions);
_allActions.AddRange(InvocationExpressionActions);
_allActions.AddRange(MemberAccessActions);
_allActions.AddRange(UsingActions);
_allActions.AddRange(ObjectCreationExpressionActions);
_allActions.AddRange(NamespaceActions);
return _allActions;
}
}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed, this wouldn't allocate 2x memory because it's a local variable that gets disposed after the getter exits. We can look into this and the same behavior in fileactions to see if there's a better way to do it. |
||
|
|
||
| } | ||
| } | ||
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.
Could you add a doc comment for the class that describes its use and how it differs from other file actions?
Also, would it make sense to rename anything with the prefix
IDEto something more generic? Dependencies shouldn't be aware of the libraries or projects that consume them, but I also don't have full context here about how this model is consumed.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.
We don't usually add doc comments for models. We can document our models classes at a later stage if needed.