Skip to content

Remove dead code from binding #393

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
JuditRose merged 1 commit into from
Feb 20, 2019
Merged

Remove dead code from binding #393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 25 additions & 26 deletions src/System.Windows.Forms/src/System/Windows/Forms/BindingSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ namespace System.Windows.Forms {
using System.Collections.Generic;
using System.ComponentModel;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Drawing;
using System.Globalization;
using System.Reflection;
Expand Down Expand Up @@ -58,7 +59,7 @@ public class BindingSource : Component,
private string dataMember = string.Empty;
private string sort = null;
private string filter = null;
private CurrencyManager currencyManager;
private readonly CurrencyManager currencyManager;
private bool raiseListChangedEvents = true;
private bool parentsCurrentItemChanging = false;
private bool disposedOrFinalized = false;
Expand Down Expand Up @@ -1412,23 +1413,23 @@ private void UnhookItemChangedEventsForOldCurrent() {
///////////////////////////////////////////////////////////////////////////////

private void WireCurrencyManager(CurrencyManager cm) {
if (cm != null) {
Copy link
Copy Markdown
Contributor Author

@hughbe hughbe Jan 29, 2019

Choose a reason for hiding this comment

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

As seen above, we can make currencyManager readonly as it is only allocated in the constructor. It can not be null

cm.PositionChanged += new EventHandler(CurrencyManager_PositionChanged);
cm.CurrentChanged += new EventHandler(CurrencyManager_CurrentChanged);
cm.CurrentItemChanged += new EventHandler(CurrencyManager_CurrentItemChanged);
cm.BindingComplete += new BindingCompleteEventHandler(CurrencyManager_BindingComplete);
cm.DataError += new BindingManagerDataErrorEventHandler(CurrencyManager_DataError);
}
Debug.Assert(cm != null);

cm.PositionChanged += new EventHandler(CurrencyManager_PositionChanged);
cm.CurrentChanged += new EventHandler(CurrencyManager_CurrentChanged);
cm.CurrentItemChanged += new EventHandler(CurrencyManager_CurrentItemChanged);
cm.BindingComplete += new BindingCompleteEventHandler(CurrencyManager_BindingComplete);
cm.DataError += new BindingManagerDataErrorEventHandler(CurrencyManager_DataError);
}

private void UnwireCurrencyManager(CurrencyManager cm) {
if (cm != null) {
Copy link
Copy Markdown
Contributor Author

@hughbe hughbe Jan 29, 2019

Choose a reason for hiding this comment

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

Same reasoning

cm.PositionChanged -= new EventHandler(CurrencyManager_PositionChanged);
cm.CurrentChanged -= new EventHandler(CurrencyManager_CurrentChanged);
cm.CurrentItemChanged -= new EventHandler(CurrencyManager_CurrentItemChanged);
cm.BindingComplete -= new BindingCompleteEventHandler(CurrencyManager_BindingComplete);
cm.DataError -= new BindingManagerDataErrorEventHandler(CurrencyManager_DataError);
}
Debug.Assert(cm != null);

cm.PositionChanged -= new EventHandler(CurrencyManager_PositionChanged);
cm.CurrentChanged -= new EventHandler(CurrencyManager_CurrentChanged);
cm.CurrentItemChanged -= new EventHandler(CurrencyManager_CurrentItemChanged);
cm.BindingComplete -= new BindingCompleteEventHandler(CurrencyManager_BindingComplete);
cm.DataError -= new BindingManagerDataErrorEventHandler(CurrencyManager_DataError);
}

private void WireDataSource() {
Expand Down Expand Up @@ -1514,17 +1515,15 @@ void ISupportInitialize.EndInit() {
}
}

// Respond to late completion of the DataSource's initialization, by completing our own initialization.
// This situation can arise if the call to the DataSource's EndInit() method comes after the call to the
// BindingSource's EndInit() method (since code-generated ordering of these calls is non-deterministic).
//
private void DataSource_Initialized(object sender, EventArgs e) {
ISupportInitializeNotification dsInit = (this.DataSource as ISupportInitializeNotification);

Debug.Assert(dsInit != null, "BindingSource: ISupportInitializeNotification.Initialized event received, but current DataSource does not support ISupportInitializeNotification!");
Copy link
Copy Markdown
Contributor Author

@hughbe hughbe Jan 29, 2019

Choose a reason for hiding this comment

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

Broken debug assertions. Can occur when changing the data source on initialisation, or when initialization fails but EndInit is called

Debug.Assert(dsInit.IsInitialized, "BindingSource: DataSource sent ISupportInitializeNotification.Initialized event but before it had finished initializing.");

if (dsInit != null) {
/// <devdoc>
/// Respond to late completion of the DataSource's initialization, by completing our own initialization.
/// This situation can arise if the call to the DataSource's EndInit() method comes after the call to the
/// BindingSource's EndInit() method (since code-generated ordering of these calls is non-deterministic).
/// </devdoc>
private void DataSource_Initialized(object sender, EventArgs e)
{
if (DataSource is ISupportInitializeNotification dsInit)
{
dsInit.Initialized -= new EventHandler(DataSource_Initialized);
}

Expand Down
93 changes: 30 additions & 63 deletions src/System.Windows.Forms/src/System/Windows/Forms/ListBindingHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ namespace System.Windows.Forms {
using System.Collections;
using System.Collections.Generic;
using System.Reflection;
using System.Diagnostics.CodeAnalysis;

/// <include file='doc\ListBindingHelper.uex' path='docs/doc[@for="ListBindingHelper"]/*' />
/// <devdoc>
Expand Down Expand Up @@ -99,8 +100,8 @@ public static string GetListName(object list, PropertyDescriptor[] listAccessors
Type type;
// We always resolve via type in this case (not an instance)
if ((null == listAccessors) || (listAccessors.Length == 0)) {
Type listAsType = list as Type;
if (listAsType != null) {
if (list is Type listAsType)
{
type = listAsType;
}
else {
Expand All @@ -109,8 +110,7 @@ public static string GetListName(object list, PropertyDescriptor[] listAccessors
}
else {
// We don't walk down - always use type name
PropertyDescriptor pd = listAccessors[0];
type = pd.PropertyType;
type = listAccessors[0].PropertyType;
}

name = GetListNameFromType(type);
Expand Down Expand Up @@ -145,33 +145,27 @@ public static PropertyDescriptorCollection GetListItemProperties(object list) {
return pdc;
}

/// <include file='doc\ListBindingHelper.uex' path='docs/doc[@for="ListBindingHelper.GetListItemProperties1"]/*' />
public static PropertyDescriptorCollection GetListItemProperties(object list, PropertyDescriptor[] listAccessors) {
PropertyDescriptorCollection pdc;

if ((null == listAccessors) || (listAccessors.Length == 0)) {
pdc = GetListItemProperties(list);
public static PropertyDescriptorCollection GetListItemProperties(object list, PropertyDescriptor[] listAccessors)
{
if (listAccessors == null || listAccessors.Length == 0)
{
return GetListItemProperties(list);
}
else {
if (list is Type) {
pdc = GetListItemPropertiesByType(list as Type, listAccessors);
}
else {
object target = GetList(list);

if (target is ITypedList) {
pdc = (target as ITypedList).GetItemProperties(listAccessors);
}
else if (target is IEnumerable) {
pdc = GetListItemPropertiesByEnumerable(target as IEnumerable, listAccessors);
}
else {
pdc = GetListItemPropertiesByInstance(target, listAccessors, 0);
}
}
else if (list is Type type)
{
return GetListItemPropertiesByType(type, listAccessors, 0);
}

return pdc;
object target = GetList(list);

if (target is ITypedList typedList) {
return typedList.GetItemProperties(listAccessors);
}
else if (target is IEnumerable enumerable) {
return GetListItemPropertiesByEnumerable(enumerable, listAccessors, 0);
Copy link
Copy Markdown
Contributor Author

@hughbe hughbe Jan 29, 2019

Choose a reason for hiding this comment

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

This is all formatting except for now we call GetListItemPropertiesByEnumerable(IEnumerable, PropertyDescriptor[], int) instead of the GetListItemPropertiesByEnumerable(IEnumerable, PropertyDescriptor[]) function which had dead code and does exactly the same thing when removing the dead code

}

return GetListItemPropertiesByInstance(target, listAccessors, 0);
}

/// <include file='doc\ListBindingHelper.uex' path='docs/doc[@for="ListBindingHelper.GetListItemProperties2"]/*' />
Expand Down Expand Up @@ -241,6 +235,7 @@ public static Type GetListItemType(object list) {
}

// Create an object of the given type. Throw an exception if this fails.
[ExcludeFromCodeCoverage]
private static object CreateInstanceOfType(Type type) {
object instancedObject = null;
Exception instanceException = null;
Expand Down Expand Up @@ -320,19 +315,6 @@ private static string GetListNameFromType(Type type) {
return name;
}

private static PropertyDescriptorCollection GetListItemPropertiesByType(Type type, PropertyDescriptor[] listAccessors) {
PropertyDescriptorCollection pdc = null;

if ((null == listAccessors) || (listAccessors.Length == 0)) {
Copy link
Copy Markdown
Contributor Author

@hughbe hughbe Jan 29, 2019

Choose a reason for hiding this comment

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

This could never be null or empty - demonstrated as this function is only called once in L165 which exits early if null or empty.

Once dead code is removed, the function can be inlined into single caller.

pdc = GetListItemPropertiesByType(type);
}
else {
pdc = GetListItemPropertiesByType(type, listAccessors, 0);
}

return pdc;
}

private static PropertyDescriptorCollection GetListItemPropertiesByType(Type type, PropertyDescriptor[] listAccessors, int startIndex) {
PropertyDescriptorCollection pdc = null;
Type subType = listAccessors[startIndex].PropertyType;
Expand Down Expand Up @@ -410,39 +392,24 @@ private static PropertyDescriptorCollection GetListItemPropertiesByEnumerable(IE
return pdc;
}

private static PropertyDescriptorCollection GetListItemPropertiesByEnumerable(IEnumerable enumerable, PropertyDescriptor[] listAccessors) {
PropertyDescriptorCollection pdc = null;

if ((null == listAccessors) || (listAccessors.Length == 0)) {
Copy link
Copy Markdown
Contributor Author

@hughbe hughbe Jan 29, 2019

Choose a reason for hiding this comment

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

This could never be null or empty - demonstrated as this function is only called once in L165 which exits early if null or empty.

Once dead code is removed, the function can be inlined into single caller.

pdc = GetListItemPropertiesByEnumerable(enumerable);
}
else {
ITypedList typedList = enumerable as ITypedList;
if (typedList != null) {
pdc = typedList.GetItemProperties(listAccessors);
}
else {
// Walk the tree
pdc = GetListItemPropertiesByEnumerable(enumerable, listAccessors, 0);
}
}
return pdc;
}

private static Type GetListItemTypeByEnumerable(IEnumerable iEnumerable) {
object instance = GetFirstItemByEnumerable(iEnumerable);
return (instance != null) ? instance.GetType() : typeof(object);
}

private static PropertyDescriptorCollection GetListItemPropertiesByInstance(object target, PropertyDescriptor[] listAccessors, int startIndex) {
private static PropertyDescriptorCollection GetListItemPropertiesByInstance(object target, PropertyDescriptor[] listAccessors, int startIndex)
{
Debug.Assert(listAccessors != null);
Copy link
Copy Markdown
Contributor Author

@hughbe hughbe Jan 29, 2019

Choose a reason for hiding this comment

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

Function only called from L168 which bails early for null or empty list accessors and GetListItemPropertiesByEnumerable which is called from L165 which as we know bailed early


// At this point, things can be simplified because:
// We know target is _not_ a list
// We have an instance

PropertyDescriptorCollection pdc;

// Get the value of the first listAccessor
if (listAccessors != null && listAccessors.Length > startIndex) {
if (listAccessors.Length > startIndex)
{
// Get the value (e.g. given Foo with property Bar, this gets Foo.Bar)
object value = listAccessors[startIndex].GetValue(target);

Expand Down Expand Up @@ -606,7 +573,7 @@ private static PropertyDescriptorCollection GetListItemPropertiesByEnumerable(IE
else {
pdc = TypeDescriptor.GetProperties(instance, BrowsableAttributeList);

if (!(enumerable is IList) && (pdc == null || pdc.Count == 0)) {
if (!(enumerable is IList) && pdc.Count == 0) {
Copy link
Copy Markdown
Contributor Author

@hughbe hughbe Jan 29, 2019

Choose a reason for hiding this comment

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

TypeDescriptor.GetProperties can never return null.

pdc = TypeDescriptor.GetProperties(enumerable, BrowsableAttributeList);
}
}
Expand Down