Run code quality analyzers in the coreclr/tools directory #74825
Conversation
tlakollo
changed the title
|
cc @dotnet/crossgen-contrib @dotnet/ilc-contrib |
src/coreclr/tools/Common/Internal/Metadata/NativeFormat/MdBinaryReaderGen.cs
Show resolved
Hide resolved
| } |
There was a problem hiding this comment.
Can you please check if this should be "ported" to linker repo as well? (Probably the setting for lint to do this for us).
There was a problem hiding this comment.
Opened https://github.com/dotnet/linker/issues/3018 to track enabling runtime code-quality analyzer on top of existing linker analyzers
| await MergeSortCore<T, T[], ArrayAccessor<T>, TComparer, TCompareAsEqualAction>.ParallelSort(localCopyOfHalfOfArray, 0, halfLen, comparer); | ||
| await rightSortTask; | ||
| await MergeSortCore<T, T[], ArrayAccessor<T>, TComparer, TCompareAsEqualAction>.ParallelSort(localCopyOfHalfOfArray, 0, halfLen, comparer).ConfigureAwait(true); | ||
| await rightSortTask.ConfigureAwait(true); |
There was a problem hiding this comment.
Why true rather than false?
There was a problem hiding this comment.
¯\_(ツ)_/¯
I'm not familiar with what is happening in the code and although less performant and deadlock prone, using ConfigureAwait(true) does virtually the same as just awaiting for it. If you believe it should be false, I can change it
There was a problem hiding this comment.
I will merge it as is for now, let me know if you want me to create a follow-up change about this :D
There was a problem hiding this comment.
It's very rare to see ConfigureAwait(true) and it's usually for a very specific reason. These should almost certainly be false.
src/coreclr/tools/Common/TypeSystem/Common/Utilities/CustomAttributeTypeNameParser.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/Common/TypeSystem/Common/Utilities/LockFreeReaderHashtable.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/Common/TypeSystem/Ecma/EcmaSignatureTranslator.cs
Outdated
Show resolved
Hide resolved
.../ILCompiler.MetadataTransform/Internal/Metadata/NativeFormat/Writer/NativeFormatWriterGen.cs
Show resolved
Hide resolved
dotnet#74825 (review) I don't think we need to run on the same thread.
#74825 (review) I don't think we need to run on the same thread.
The `readonly` addition was one of the 3600 diffs in dotnet#74825. `MarkStrategy` can have mutable state that was getting lost because Roslyn considers it valid to put `_marker` into a temporary before calling instance methods on it. This resulted in DependencyGraphViewer hanging after seeing tens of thousands of new graphs being added.
The `readonly` addition was one of the 3600 diffs in #74825. `MarkStrategy` can have mutable state that was getting lost because Roslyn considers it valid to put `_marker` into a temporary before calling instance methods on it. This resulted in DependencyGraphViewer hanging after seeing tens of thousands of new graphs being added.
ghost
locked as resolved and limited conversation to collaborators
Adds the <RunAnalyzers> property set to true into the different ilc projects
Adds some custom configurations into the editorconfig of ilc