Page MenuHomePhabricator

C#: CA1051: Do not declare visible instance fields
Open, NormalPublic

Description

Probably some ownership bool, handle, etc are "leaking" as field instead of properties.

lauromoura triaged this task as Normal priority.

CA1051 is applied both to structs and classes.

At first I directed Bruno to suppress the warning for this error in the structs, as he did in D10553, as replacing the fields with properties could make it impossible to use such members as out and ref parameters.

But since we are moving towards a (multi-valued) property world, this should not be an issue.

Except for the cost of indirection (calling the get/set block), althought MS seems to claim the compiler would be smart enough to inline simple blocks.

@segfaultxavi @YOhoho @woohyun what do you think?

Why would "replacing the fields with properties could make it impossible to use such members as out and ref parameters" ? I do not understand the problem.

Why would "replacing the fields with properties could make it impossible to use such members as out and ref parameters" ? I do not understand the problem.

For example: https://dotnetfiddle.net/9IGwej

Eina.Position2d pos = new Eina.Position2D();
Foo.GetPos(out pos.X, pos.Y); // KA-BOOMS if `pos.X` or `pos.Y` is a property

This should not be much an issue as the "natural" API using Position2D would return the entire position as a whole, no? GetPos(out Eina.Position2D pos)

Gotta love C#.

Indeed, we should use compound types (Position2D, Vector2D, Size2D...) as much as possible and move away from cx, cy, and company.

But there might be cases where this cannot be done and the warning has to be silenced?

Gotta love C#.

Indeed, we should use compound types (Position2D, Vector2D, Size2D...) as much as possible and move away from cx, cy, and company.

Agreed.

But there might be cases where this cannot be done and the warning has to be silenced?

One such case is native structs, which are used for marshaling. But since D10588 they are internal, so this should not be an issue for them.