Skip to content

Remove using empty base AO ctor#6571

Merged
1 commit merged intodotnet:mainfrom
lisoenot:Removing-inherit-empty-AO-ctor
Feb 1, 2022
Merged

Remove using empty base AO ctor#6571
1 commit merged intodotnet:mainfrom
lisoenot:Removing-inherit-empty-AO-ctor

Conversation

@lisoenot
Copy link
Contributor

@lisoenot lisoenot commented Jan 28, 2022 •
edited
Loading

Resolve comment from pr #6432

Proposed changes

  • Remove call empty ctor of Accessible Object class in inherited classes.

Customer Impact

  • No

Regression?

  • No

Risk

  • No
Microsoft Reviewers: Open in CodeFlow

@lisoenot lisoenot requested a review from a team as a code owner January 28, 2022 17:42
@ghost ghost assigned lisoenot Jan 28, 2022
@lisoenot lisoenot mentioned this pull request Jan 28, 2022
Merged
@RussKie
Copy link
Contributor

RussKie commented Jan 28, 2022

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 28, 2022

Azure Pipelines successfully started running 2 pipeline(s).

WPMGPRoSToTeMa
WPMGPRoSToTeMa previously approved these changes Jan 29, 2022
View reviewed changes
src/System.Windows.Forms/src/System/Windows/Forms/DataGridView.TopRowAccessibleObject.cs
Comment on lines +19 to 21
public DataGridViewTopRowAccessibleObject()
{
}
Copy link
Contributor

@WPMGPRoSToTeMa WPMGPRoSToTeMa Jan 29, 2022

Choose a reason for hiding this comment

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

Can we also remove the parameterless constructor?

@WPMGPRoSToTeMa
Copy link
Contributor

WPMGPRoSToTeMa commented Jan 29, 2022 •
edited
Loading

It's safe to remove : base() even when it calls non-empty parameterless constructor because it's called implicitly:

In a derived class, if a base-class constructor is not called explicitly by using the base keyword, the parameterless constructor, if there is one, is called implicitly.
https://docs.microsoft.com/dotnet/csharp/programming-guide/classes-and-structs/using-constructors

The only question whether we want be explicit about calling base parameterless constructors or not (and can we enforce it through code style rules in Roslyn).

cc: @RussKie

vladimir-krestov
vladimir-krestov previously approved these changes Jan 29, 2022
View reviewed changes
Copy link
Contributor

@vladimir-krestov vladimir-krestov left a comment

Choose a reason for hiding this comment

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

Approved ✔️

@RussKie
Copy link
Contributor

RussKie commented Jan 31, 2022

The default .ctors are called implicitly if those are present and no other .ctor is explicitly called.

RussKie
RussKie reviewed Jan 31, 2022
View reviewed changes
Copy link
Contributor

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

:shipit: once the comment is addressed.

@lisoenot lisoenot force-pushed the Removing-inherit-empty-AO-ctor branch 2 times, most recently from 2383799 to 69a6939 Compare January 31, 2022 12:25
@WPMGPRoSToTeMa
Copy link
Contributor

WPMGPRoSToTeMa commented Jan 31, 2022

The default .ctors are called implicitly if those are present and no other .ctor is explicitly called.

I mean is there a code style rule that will warn when the base parameterless .ctor is called explicitly? (to avoid this in future)

Tanya-Solyanik
Tanya-Solyanik previously approved these changes Jan 31, 2022
View reviewed changes
@Tanya-Solyanik Tanya-Solyanik added the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Jan 31, 2022
@Tanya-Solyanik
Copy link
Contributor

Tanya-Solyanik commented Jan 31, 2022

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 31, 2022

Azure Pipelines successfully started running 2 pipeline(s).

@RussKie
Copy link
Contributor

RussKie commented Jan 31, 2022

The default .ctors are called implicitly if those are present and no other .ctor is explicitly called.

I mean is there a code style rule that will warn when the base parameterless .ctor is called explicitly? (to avoid this in future)

The compiler will tell you :)

If there is no .ctor defined - one is implicitly added and called. If there is a non-default .ctor (i.e. with parameters) - it will always be called, no default .ctor will be generated.

RussKie
RussKie suggested changes Jan 31, 2022
View reviewed changes
src/System.Windows.Forms/src/PublicAPI.Shipped.txt
System.Windows.Forms.DataGridView.DataGridViewAccessibleObject.DataGridViewAccessibleObject(System.Windows.Forms.DataGridView! owner) -> void
System.Windows.Forms.DataGridView.DataGridViewControlCollection
System.Windows.Forms.DataGridView.DataGridViewTopRowAccessibleObject
System.Windows.Forms.DataGridView.DataGridViewTopRowAccessibleObject.DataGridViewTopRowAccessibleObject() -> void
Copy link
Contributor

@RussKie RussKie Jan 31, 2022

Choose a reason for hiding this comment

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

We must not alter the public API surface. See https://docs.microsoft.com/dotnet/api/system.windows.forms.datagridview.datagridviewtoprowaccessibleobject.-ctor

Copy link
Contributor Author

@lisoenot lisoenot Feb 1, 2022

Choose a reason for hiding this comment

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

Ok, restored paramless ctor and tis file. Thanks

@ghost ghost added the waiting-author-feedback The team requires more information from the author label Jan 31, 2022
@WPMGPRoSToTeMa
Copy link
Contributor

WPMGPRoSToTeMa commented Jan 31, 2022 •
edited
Loading

The compiler will tell you :)

If there is no .ctor defined - one is implicitly added and called. If there is a non-default .ctor (i.e. with parameters) - it will always be called, no default .ctor will be generated.

This is what I actually meant (code style rule, not the code generation):

public DataGridViewTopRowAccessibleObject() : base()
                                              ^~~~~~
                                              |
                                              Warning: base parameterless constructor is called explicitly, please don't do that

And as I can see there is no rule for that :(

@lisoenot lisoenot force-pushed the Removing-inherit-empty-AO-ctor branch from 69a6939 to df4f7b4 Compare February 1, 2022 06:19
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Feb 1, 2022
@RussKie RussKie added :octocat: automerge code cleanup cleanup code for unused apis/properties/comments - no functional changes. and removed ready-to-merge PRs that are ready to merge but worth notifying the internal team. labels Feb 1, 2022
@ghost
Copy link

ghost commented Feb 1, 2022

Hello @RussKie!

Because this pull request has the :octocat: automerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost
ghost approved these changes Feb 1, 2022
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Happy to oblige

@ghost ghost merged commit 2be4092 into dotnet:main Feb 1, 2022
@ghost ghost added this to the 7.0 Preview2 milestone Feb 1, 2022
@lisoenot lisoenot deleted the Removing-inherit-empty-AO-ctor branch February 2, 2022 13:17
@ghost ghost locked as resolved and limited conversation to collaborators Mar 4, 2022
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dreddy-work dreddy-work Awaiting requested review from dreddy-work

5 more reviewers

@RussKie RussKie RussKie approved these changes

@WPMGPRoSToTeMa WPMGPRoSToTeMa WPMGPRoSToTeMa left review comments

@Tanya-Solyanik Tanya-Solyanik Tanya-Solyanik left review comments

@vladimir-krestov vladimir-krestov vladimir-krestov left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@lisoenot lisoenot

Labels

code cleanup cleanup code for unused apis/properties/comments - no functional changes. :octocat: automerge

Projects

None yet

Milestone

7.0 Preview2

Development

Successfully merging this pull request may close these issues.

5 participants

@lisoenot @RussKie @WPMGPRoSToTeMa @Tanya-Solyanik @vladimir-krestov