-
Notifications
You must be signed in to change notification settings - Fork 134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Call-graph based Unity.Burst support #1665
Conversation
2. fixed unityProblemAnalyzerContext switches 3. sdk compilation fixes
…on based on recursive processor
2. test fixes 3. unity jobs defining 4. test attribute mark added
...-unity/src/CSharp/Daemon/Stages/BurstCodeAnalysis/Analyzers/BurstForeachStatementAnalyzer.cs
Outdated
Show resolved
Hide resolved
...nity/src/CSharp/Daemon/Stages/BurstCodeAnalysis/Analyzers/BurstStringLiteralOwnerAnalyzer.cs
Outdated
Show resolved
Hide resolved
...er/resharper-unity/test/data/CSharp/Daemon/Stages/BurstCodeAnalysis/resharper/ComplexTest.cs
Outdated
Show resolved
Hide resolved
...er/resharper-unity/test/data/CSharp/Daemon/Stages/BurstCodeAnalysis/resharper/ComplexTest.cs
Outdated
Show resolved
Hide resolved
...rp/Daemon/Stages/PerformanceCriticalCodeAnalysis/CallGraph/ExpensiveCodeCallGraphAnalyzer.cs
Outdated
Show resolved
Hide resolved
...s/PerformanceCriticalCodeAnalysis/CallGraph/PerformanceCriticalCodeCallGraphMarksProvider.cs
Show resolved
Hide resolved
...er-unity/src/CSharp/Daemon/Stages/BurstCodeAnalysis/CallGraph/CallGraphBurstMarksProvider.cs
Outdated
Show resolved
Hide resolved
resharper/resharper-unity/src/CSharp/Daemon/Stages/BurstCodeAnalysis/BurstCodeAnalysisUtil.cs
Show resolved
Hide resolved
resharper/resharper-unity/src/CSharp/Daemon/Stages/BurstCodeAnalysis/BurstCodeAnalysisUtil.cs
Outdated
Show resolved
Hide resolved
2. test additions and fixes 3. multiple PerformanceCriticalCodeCallGraphMarksProvider.cs and ExpensiveCodeCallGraphAnalyzer.cs fixes and enhancements 4. burst ui 5.
2. BurstCodeAnalysisUtil.cs refactoring and clarifying
…nalysis possible flicks
…vent spreading burst context
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.
- I'm not sure if these new highlights should be configurable severity. If they're compiler errors, can't be disabled, and will break compilation, we should treat them as static severity. We can always disable analysis in options if there are problems
- We shouldn't show the compiler ID as part of the warning - we don't do this anywhere else, e.g. for C# compiler errors/warnings, although I think we can associate a highlight with. It also makes it harder to read the code to know what the highlight is about.
- Replace the backtick
</code> with single quote
'` to match other R#/Rider messages - Remove all full stops from the end of
<Message>
attributes. We don't end tooltips with a full stop.
Are the tooltip messages/descriptions taken from the compiler error messages? We might want to do something a little more user friendly, like we do with C# compiler messages.
resharper/resharper-unity/src/Application/UI/Options/VisualStudio/ReSharperOptionsPage.cs
Outdated
Show resolved
Hide resolved
{ | ||
public CallGraphBurstMarksProvider(ISolution solution) | ||
: base(nameof(CallGraphBurstMarksProvider), | ||
new CallGraphOutcomingPropagator(solution, nameof(CallGraphBurstMarksProvider))) |
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.
I know it's an SDK method, but should Outcoming
be Outgoing
?
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.
Outcoming
was chosen to show that it is opposite toIncomingPropagator
, so idk if this is correct change
|
||
<SeverityConfiguration> | ||
<Group name="UnityHighlightingGroupIds.Burst"> | ||
<Tag externalName="BC1001Error.HIGHLIGHTING_ID" default="WARNING"> |
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.
Should this be an error highlight instead of a warning? And unless these can be disabled, shouldn't they be static highlights?
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.
Burst compiler is a special case: if compiled at the editor - despite unsuccessful compilation unity project will be executed, if built as standalone - unity will consider it as a normal error, so I think warning is the best severity, let the user decide if this is critical for him
resharper/resharper-unity/src/CSharp/Daemon/Stages/Highlightings/UnityHighlightingGroupIds.cs
Outdated
Show resolved
Hide resolved
resharper/resharper-unity/src/CSharp/Daemon/Stages/Highlightings/UnityHighlightingGroupIds.cs
Outdated
Show resolved
Hide resolved
<SeverityConfiguration> | ||
<Group name="UnityHighlightingGroupIds.Burst"> | ||
<Tag externalName="BC1001Error.HIGHLIGHTING_ID" default="WARNING"> | ||
<Title>BC1001</Title> |
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.
I think these titles need more information - I think they're used to display the warning in the Inspection Severity page, and should reflect the tooltip, rather than the compiler ID
resharper/resharper-unity/src/CSharp/Daemon/Errors/BurstErrors.xml
Outdated
Show resolved
Hide resolved
3f413d6
to
acdc899
Compare
acdc899
to
e74c698
Compare
…arper-unity into net202-kononov-cg
Support for Unity.Burst
highlights access to managed objects, object creation etc
transitively propagates burst contexts
highlighting are taken and annotated to match Burst compiler