Private data members

Private data members

[Vladimir Prus]

I have run into a case when it's desirable that all modifications
of a certain structure field in GDB codebase go via function that
can enforce necessary invariants. Specifically, the ignore_count
in struct breakpoint does not make any sense for tracepoint, therefore
I want to introduce a function that will throw if non-zero ignore count
is ever set for a tracepoint.

At the moment, there are at least 3 places that directly assign
a value to that field, and while I can convert them easily, nothing
will prevent a direct assignment to appear in future. In C++,
one would use 'private' visibility for that member, but it's not
available in C. So, how about introducing a small convention --
that members with names ending in '_' are 'private' and should
never be accessed by outside code. Another alternative is to
modify the comment on ignore_count, but that is much more likely
to be ignored.

Comments?

- Volodya

Re: Private data members

[Daniel Jacobowitz]

On Wed, Jul 29, 2009 at 12:45:32PM +0400, Vladimir Prus wrote:
> At the moment, there are at least 3 places that directly assign
> a value to that field, and while I can convert them easily, nothing
> will prevent a direct assignment to appear in future. In C++,
> one would use 'private' visibility for that member, but it's not
> available in C. So, how about introducing a small convention --
> that members with names ending in '_' are 'private' and should
> never be accessed by outside code. Another alternative is to
> modify the comment on ignore_count, but that is much more likely
> to be ignored.

How about struct { int ignore_count; } private; ?  I don't think
ignore_count_ is clear.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Private data members

[Vladimir Prus]

Daniel Jacobowitz wrote:

> On Wed, Jul 29, 2009 at 12:45:32PM +0400, Vladimir Prus wrote:
>> At the moment, there are at least 3 places that directly assign
>> a value to that field, and while I can convert them easily, nothing
>> will prevent a direct assignment to appear in future. In C++,
>> one would use 'private' visibility for that member, but it's not
>> available in C. So, how about introducing a small convention --
>> that members with names ending in '_' are 'private' and should
>> never be accessed by outside code. Another alternative is to
>> modify the comment on ignore_count, but that is much more likely
>> to be ignored.
> 
> How about struct { int ignore_count; } private; ?

Might work.

> I don't think ignore_count_ is clear.

Well, it's a convention for naming private variables used by roughly
half of C++ developers, so I expect many current and future contributors
know it already.

- Volodya

Re: Private data members

[Tom Tromey]

>>>>> "Volodya" == Vladimir Prus <vladimir@codesourcery.com> writes:

Volodya> I have run into a case when it's desirable that all
Volodya> modifications of a certain structure field in GDB codebase go
Volodya> via function that can enforce necessary invariants.

Volodya> At the moment, there are at least 3 places that directly assign
Volodya> a value to that field, and while I can convert them easily,
Volodya> nothing will prevent a direct assignment to appear in future.

In the past somebody solved this same problem for struct value by
putting the struct definition into value.c and adding a bunch of
accessors.

I'm lukewarm about this technique, but it can be made to work.

Daniel's suggested approach would also be fine with me.

If you want a real guarantee, you could write an spatch script to detect
assignments to this field, and run it periodically.

Tom

Re: Private data members

[Samuel Bronson]

At Wed, 29 Jul 2009 17:41:20 +0400,
Vladimir Prus wrote:
> 
> Daniel Jacobowitz wrote:
> >
> > I don't think ignore_count_ is clear.
> 
> Well, it's a convention for naming private variables used by roughly
> half of C++ developers, so I expect many current and future contributors
> know it already.

Hmm. I'm only familiar with the _ignore_count convention that's used
in e.g. Python (and sometimes in C, even though it's not really
allowed in user code there).

Re: Private data members

[Vladimir Prus]

On Wednesday 29 July 2009 Samuel Bronson wrote:

> At Wed, 29 Jul 2009 17:41:20 +0400,
> Vladimir Prus wrote:
> > 
> > Daniel Jacobowitz wrote:
> > >
> > > I don't think ignore_count_ is clear.
> > 
> > Well, it's a convention for naming private variables used by roughly
> > half of C++ developers, so I expect many current and future contributors
> > know it already.
> 
> Hmm. I'm only familiar with the _ignore_count convention that's used
> in e.g. Python (and sometimes in C, even though it's not really
> allowed in user code there).

That's why I've said "half". Though I actually expected the other
half to be familiar with the m_foo convention :-)

- Volodya

Re: Private data members

[Michael Snyder]

Tom Tromey wrote:
>>>>>> "Volodya" == Vladimir Prus <vladimir@codesourcery.com> writes:
> 
> Volodya> I have run into a case when it's desirable that all
> Volodya> modifications of a certain structure field in GDB codebase go
> Volodya> via function that can enforce necessary invariants.
> 
> Volodya> At the moment, there are at least 3 places that directly assign
> Volodya> a value to that field, and while I can convert them easily,
> Volodya> nothing will prevent a direct assignment to appear in future.
> 
> In the past somebody solved this same problem for struct value by
> putting the struct definition into value.c and adding a bunch of
> accessors.
> 
> I'm lukewarm about this technique, but it can be made to work.
> 
> Daniel's suggested approach would also be fine with me.

How about combining the two?

Define struct breakpoint in breakpoint.h, put any public members
in there, and then include a member "struct breakpoint_private"
that is only a forward declaration.

Then define "struct breakpoint_private" in breakpoint.c, so that
its members will be invisible outside of that module.

Re: Private data members

[Andreas Schwab]

Michael Snyder <msnyder@vmware.com> writes:

> Define struct breakpoint in breakpoint.h, put any public members
> in there, and then include a member "struct breakpoint_private"
> that is only a forward declaration.

That only works if this member is a pointer.  You cannot have struct
members with incomplete types.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Private data members

[Michael Snyder]

Andreas Schwab wrote:
> Michael Snyder <msnyder@vmware.com> writes:
> 
>> Define struct breakpoint in breakpoint.h, put any public members
>> in there, and then include a member "struct breakpoint_private"
>> that is only a forward declaration.
> 
> That only works if this member is a pointer.  You cannot have struct
> members with incomplete types.

Right.  But that can still be done, because we have a local allocator.
Breakpoint structs are created by set_raw_breakpoint_without_location,
a static function, and the exported interface is set_raw_breakpoint.

As long as nobody outside of breakpoint.c allocates a struct bp,
I don't see why it shouldn't work.