Skip to content

CA1836 Prefer IsEmpty over Count #7185

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.

Sign up for GitHub

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

Merged
Forgind merged 11 commits into from
Jan 11, 2022
Merged

CA1836 Prefer IsEmpty over Count #7185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e39bbf5
CA1836 Prefer IsEmpty over Count
elachlan Dec 30, 2021
cb17b43
Update src/Build/BackEnd/BuildManager/BuildManager.cs
elachlan Dec 30, 2021
c91e0fe
Update src/Build/BackEnd/BuildManager/BuildManager.cs
elachlan Dec 30, 2021
14e50be
Revert CodeAnalysis.ruleset
elachlan Jan 8, 2022
a0ec95a
merge master
elachlan Jan 8, 2022
bb12727
enable warning on CA1836
elachlan Jan 8, 2022
ecff315
Fix remaining occurrences of CA1836 violations
elachlan Jan 8, 2022
f17563d
Merge branch 'CA1836' of github.com:elachlan/msbuild into CA1836
elachlan Jan 8, 2022
8b78e45
convert !.IsEmpty to .Any()
elachlan Jan 10, 2022
69e207e
Change from code review
elachlan Jan 11, 2022
c71ba2c
Fix Additional usages of IsEmpty
elachlan Jan 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion eng/Common.globalconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ dotnet_diagnostic.CA1834.severity = suggestion
dotnet_diagnostic.CA1835.severity = suggestion

# Prefer IsEmpty over Count
dotnet_diagnostic.CA1836.severity = suggestion
dotnet_diagnostic.CA1836.severity = warning

# Use 'Environment.ProcessId'
dotnet_diagnostic.CA1837.severity = suggestion
Expand Down
2 changes: 1 addition & 1 deletion src/Build.UnitTests/ProjectCache/ProjectCacheTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public ProjectCacheTests(ITestOutputHelper output)
_env = TestEnvironment.Create(output);

BuildManager.ProjectCacheItems.ShouldBeEmpty();
_env.WithInvariant(new CustomConditionInvariant(() => BuildManager.ProjectCacheItems.Count == 0));
_env.WithInvariant(new CustomConditionInvariant(() => BuildManager.ProjectCacheItems.IsEmpty));
}

public void Dispose()
Expand Down
4 changes: 2 additions & 2 deletions src/Build/BackEnd/BuildManager/BuildManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1202,7 +1202,7 @@ bool ProjectCacheIsPresent()

private static bool ProjectCachePresentViaVisualStudioWorkaround()
{
return BuildEnvironmentHelper.Instance.RunningInVisualStudio && ProjectCacheItems.Count > 0;
return BuildEnvironmentHelper.Instance.RunningInVisualStudio && ProjectCacheItems.Any();
}

// Cache requests on configuration N do not block future build submissions depending on configuration N.
Expand Down Expand Up @@ -1261,7 +1261,7 @@ private ProjectCacheService GetProjectCacheService()
private void AutomaticallyDetectAndInstantiateProjectCacheServiceForVisualStudio()
{
if (BuildEnvironmentHelper.Instance.RunningInVisualStudio &&
ProjectCacheItems.Count > 0 &&
ProjectCacheItems.Any() &&
_projectCacheService == null &&
_buildParameters.ProjectCacheDescriptor == null)
{
Expand Down
2 changes: 1 addition & 1 deletion src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ public void ContinueRequestWithResources(ResourceResponse response)
{
ErrorUtilities.VerifyThrow(HasActiveBuildRequest, "Request not building");
ErrorUtilities.VerifyThrow(!_terminateEvent.WaitOne(0), "Request already terminated");
ErrorUtilities.VerifyThrow(!_pendingResourceRequests.IsEmpty, "No pending resource requests");
ErrorUtilities.VerifyThrow(_pendingResourceRequests.Any(), "No pending resource requests");
VerifyEntryInActiveOrWaitingState();

_pendingResourceRequests.Dequeue()(response);
Expand Down
5 changes: 3 additions & 2 deletions src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
using ElementLocation = Microsoft.Build.Construction.ElementLocation;
using BuildAbortedException = Microsoft.Build.Exceptions.BuildAbortedException;
using TaskItem = Microsoft.Build.Execution.ProjectItemInstance.TaskItem;
using System.Linq;

#nullable disable

Expand Down Expand Up @@ -405,7 +406,7 @@ private async Task ProcessTargetStack(ITaskBuilder taskBuilder)
(
!_cancellationToken.IsCancellationRequested &&
!stopProcessingStack &&
!_targetsToBuild.IsEmpty
_targetsToBuild.Any()
)
{
TargetEntry currentTargetEntry = _targetsToBuild.Peek();
Expand Down Expand Up @@ -613,7 +614,7 @@ private void PopDependencyTargetsOnTargetFailure(TargetEntry topEntry, TargetRes
// Pop down to our parent, since any other dependencies our parent had should no longer
// execute. If we encounter an error target on the way down, also stop since the failure
// of one error target in a set declared in OnError should not cause the others to stop running.
while ((!_targetsToBuild.IsEmpty) && (_targetsToBuild.Peek() != topEntry.ParentEntry) && !_targetsToBuild.Peek().ErrorTarget)
while ((_targetsToBuild.Any()) && (_targetsToBuild.Peek() != topEntry.ParentEntry) && !_targetsToBuild.Peek().ErrorTarget)
{
TargetEntry entry = _targetsToBuild.Pop();
entry.LeaveLegacyCallTargetScopes();
Expand Down
4 changes: 2 additions & 2 deletions src/Build/Evaluation/LazyItemEvaluator.RemoveOperation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public RemoveOperation(RemoveOperationBuilder builder, LazyItemEvaluator<P, I, M
new BuildEventFileInfo(string.Empty),
"OM_MatchOnMetadataIsRestrictedToReferencedItems");

if (!_matchOnMetadata.IsEmpty)
if (_matchOnMetadata.Any())
{
_metadataSet = new MetadataTrie<P, I>(builder.MatchOnMetadataOptions, _matchOnMetadata, _itemSpec);
}
Expand All @@ -49,7 +49,7 @@ protected override void ApplyImpl(OrderedItemDataCollection.Builder listBuilder,
return;
}

bool matchingOnMetadata = !_matchOnMetadata.IsEmpty;
bool matchingOnMetadata = _matchOnMetadata.Any();
if (!matchingOnMetadata)
{
if (ItemspecContainsASingleBareItemReference(_itemSpec, _itemElement.ItemType))
Expand Down
2 changes: 1 addition & 1 deletion src/Build/Logging/ProfilerLogger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ internal ProfilerResult GetAggregatedResult(bool pruneSmallItems = true)
// So keeping that map here
var originalLocations = new Dictionary<EvaluationLocation, EvaluationLocation>(EvaluationLocationIdAgnosticComparer.Singleton);

while (!_profiledResults.IsEmpty)
Copy link
Copy Markdown
Member

@rainersigwald rainersigwald Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Forgind when we were talking about this did we talk about .Any() possibly being worse than ! .IsEmpty because the former is an extension method that might have higher overhead?

Method ActuallyEmpty Mean Error StdDev Allocated
IsEmpty False 2.484 ns 0.0534 ns 0.0473 ns -
Any False 1,361.709 ns 5.7049 ns 5.0573 ns -
IsEmpty True 178.694 ns 1.1384 ns 1.0649 ns -
Any True 177.720 ns 1.7098 ns 1.5993 ns - 
using System;
using System.Collections.Concurrent;
using System.Linq;

using BenchmarkDotNet;
using BenchmarkDotNet.Attributes;

namespace any_count;

[MemoryDiagnoser]
public class Benchmarks
{
    public static ConcurrentDictionary<string, string> emptyDictionary = new ConcurrentDictionary<string, string>();

    public static ConcurrentDictionary<string, string> nonEmptyDictionary = new ConcurrentDictionary<string, string>();

    static Benchmarks()
    {
        for (int i = 0; i < 100; i++)
        {
            nonEmptyDictionary.TryAdd(i.ToString(), i.ToString());
        }
    }

    [Params(true, false)]
    public bool ActuallyEmpty;

    [Benchmark]
    public bool IsEmpty() => (ActuallyEmpty? emptyDictionary : nonEmptyDictionary).IsEmpty;

    [Benchmark]
    public bool Any() => (ActuallyEmpty? emptyDictionary : nonEmptyDictionary).Any();
}

Copy link
Copy Markdown
Contributor

@Forgind Forgind Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Forgind when we were talking about this did we talk about .Any() possibly being worse than ! .IsEmpty because the former is an extension method that might have higher overhead?

Oh, I'm sorry; I didn't add a new comment after that part, and it slipped my mind. So do you think it's worth replacing .Any() with !.IsEmpty? (If so, sorry elachlan...)

Copy link
Copy Markdown
Contributor Author

@elachlan elachlan Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Forgind when we were talking about this did we talk about .Any() possibly being worse than ! .IsEmpty because the former is an extension method that might have higher overhead?

Oh, I'm sorry; I didn't add a new comment after that part, and it slipped my mind. So do you think it's worth replacing .Any() with !.IsEmpty? (If so, sorry elachlan...)

I am happy to. I will make another PR and replace all any calls where I can. That time difference is massive.

while (_profiledResults.Any())
{
ProfilerResult profiledResult;
var result = _profiledResults.TryDequeue(out profiledResult);
Expand Down
2 changes: 1 addition & 1 deletion src/Shared/RegisteredTaskObjectCacheBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public object UnregisterTaskObject(object key, RegisteredTaskObjectLifetime life
protected bool IsCollectionEmptyOrUncreated(RegisteredTaskObjectLifetime lifetime)
{
var collection = GetCollectionForLifetime(lifetime, dontCreate: true);
return (collection == null) || (collection.Count == 0);
return (collection == null) || collection.IsEmpty;
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/Tasks/GetSDKReferenceFiles.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ internal bool Execute(GetAssemblyName getAssemblyName, GetAssemblyRuntimeVersion

GenerateOutputItems();

if (_exceptions.Count > 0 && LogCacheFileExceptions)
if (_exceptions.Any() && LogCacheFileExceptions)
{
foreach (string exceptionMessage in _exceptions)
{
Expand Down