You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2005/10/11 16:48:43 UTC
Re: svn commit: r16652 - trunk/subversion/include
djames@tigris.org writes:
> --- trunk/subversion/include/svn_opt.h (original)
> +++ trunk/subversion/include/svn_opt.h Tue Oct 11 10:01:32 2005
> @@ -202,10 +202,12 @@
> };
>
> /**
> - * A revision value, which can be stored in one of @c svn_opt_revision_kind
> - * ways.
> + * A revision value, which can be specified as a number or a date.
> + * @note This union should only be used in a @c svn_opt_revision_t struct.
> + * @since New in 1.3.
> */
> -typedef union svn_opt_revision_value {
> +typedef union svn_opt_revision_value
> +{
> svn_revnum_t number;
> apr_time_t date;
> } svn_opt_revision_value;
Okay, thanks, ignore my earlier mail about the "@since New in 1.3" then.
:-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r16652 - trunk/subversion/include
Posted by "C. Michael Pilato" <cm...@collab.net>.
Julian Foad <ju...@btopenworld.com> writes:
> So I'd mildly suggest leaving it public.
Works for me.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r16652 - trunk/subversion/include
Posted by Julian Foad <ju...@btopenworld.com>.
C. Michael Pilato wrote:
> So, I'd like to see:
>
> - s/svn_opt_revision_value/svn_opt__revision_value_t/ (we can
> "publicize" it when we really need to).
I can't see much reason to make it public, nor much reason to make it private.
In that case it should be private, because that gives us more flexibility in
the future.
On the other hand, trying to make a part of a public structure private is
probably not going to work very well in a vague hand-waving whole-experience
way. Like giving a bit of extra hassle to SWIG maintainers and other such
tools: they may run into problems if they try to say "this tool should ignore
double-underscore symbols".
I don't know. In general I think we should avoid making too many things
public, but I think we've gone way beyond the stage where one more makes any
difference. (In other words, our APIs have much bigger over-exposure problems
than this.)
So I'd mildly suggest leaving it public.
> - the definition be like "typedef union svn_opt... { } svn_opt...;"
Indeed.
I think it's time to just do it now, whichever way you finally feel like.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r16652 - trunk/subversion/include
Posted by "C. Michael Pilato" <cm...@collab.net>.
So, I'd like to see:
- s/svn_opt_revision_value/svn_opt__revision_value_t/ (we can
"publicize" it when we really need to).
- the definition be like "typedef union svn_opt... { } svn_opt...;"
Julian Foad <ju...@btopenworld.com> writes:
> > On Tue, 11 Oct 2005, kfogel@collab.net wrote:
> >
> >>Why is it not 'svn_opt_revision_value_t'? I thought we always used _t
> >>for types that are unaccompanied by some other type-indicating keyword
> >>(e.g., 'struct' or 'enum'). We should settle this now, since this is
> >>going into 1.3.
>
> We use "_t" consistently for structs. Most enums have no suffix, some
> have "_t", and at least one has "_e". This is our first public
> union. (Congratulations are in order :-)
>
> I favour adding a "_t" suffix.
>
> Daniel Rall wrote:
> > I too prefer the _t suffix, which is consistent with our struct naming
> > convention. While we're at it, why bother naming the enum when it's just
> > going to be typedef'd (e.g. does SWIG need that)? Leaving it anonymous
> > results in less code to read. My $0.02 USD.
>
> It is general good practice (in my world) to give every struct (and
> union etc.) a name and a typedef name that are identical: "typedef
> struct foo_t { ...} foo_t;". To declare the existence of it in order
> to have pointers to it, you need to say "struct foo_t;", and in all
> other usage it is more convenient to refer to it as just "foo_t".
>
> In Subversion we do this very consistently for structs, very
> inconsistently for enums, and there are only two unions in the public
> headers (this and another anonymous one).
>
> - Julian
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r16652 - trunk/subversion/include
Posted by Julian Foad <ju...@btopenworld.com>.
> On Tue, 11 Oct 2005, kfogel@collab.net wrote:
>
>>Why is it not 'svn_opt_revision_value_t'? I thought we always used _t
>>for types that are unaccompanied by some other type-indicating keyword
>>(e.g., 'struct' or 'enum'). We should settle this now, since this is
>>going into 1.3.
We use "_t" consistently for structs. Most enums have no suffix, some have
"_t", and at least one has "_e". This is our first public union.
(Congratulations are in order :-)
I favour adding a "_t" suffix.
Daniel Rall wrote:
> I too prefer the _t suffix, which is consistent with our struct naming
> convention. While we're at it, why bother naming the enum when it's just
> going to be typedef'd (e.g. does SWIG need that)? Leaving it anonymous
> results in less code to read. My $0.02 USD.
It is general good practice (in my world) to give every struct (and union etc.)
a name and a typedef name that are identical: "typedef struct foo_t { ...}
foo_t;". To declare the existence of it in order to have pointers to it, you
need to say "struct foo_t;", and in all other usage it is more convenient to
refer to it as just "foo_t".
In Subversion we do this very consistently for structs, very inconsistently for
enums, and there are only two unions in the public headers (this and another
anonymous one).
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[OT] Re: svn commit: r16652 - trunk/subversion/include
Posted by Daniel Rall <dl...@finemaltcoding.com>.
On Wed, 12 Oct 2005, David James wrote:
...
> In addition to this, I also personally find definitions easier to read
> when they are named explicitly. Here's an example that shows why:
> typedef struct svn_foo_bar_t {
> ... 100 lines of member definitions ..
> } svn_foo_bar_t;
>
> (If it weren't for the explicit "svn_foo_bar_t" at the top of the
> definition, I'd have to scan through 100 lines of member definitions
> to find the name of the struct.)
My editor tells me that information. Additionally, I've developed a habit of
moving to the end of the struct/class/whatever (open brace to close brace).
*shrug*
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r16652 - trunk/subversion/include
Posted by David James <ja...@gmail.com>.
On 10/11/05, Daniel Rall <dl...@finemaltcoding.com> wrote:
> On Tue, 11 Oct 2005, kfogel@collab.net wrote:
> ...
> > Why is it not 'svn_opt_revision_value_t'? I thought we always used _t
> > for types that are unaccompanied by some other type-indicating keyword
> > (e.g., 'struct' or 'enum'). We should settle this now, since this is
> > going into 1.3.
>
> I too prefer the _t suffix, which is consistent with our struct naming
> convention. While we're at it, why bother naming the enum when it's just
> going to be typedef'd (e.g. does SWIG need that)? Leaving it anonymous
> results in less code to read. My $0.02 USD.
Unfortunately, SWIG does not recognize anonymous structs, enums, or
unions, even if they are typedef'd. The same rule applies to our SWIG
header-wrapper generator. So, from a SWIG perspective, it's better if
we stick to our current convention of naming our datatypes.
In addition to this, I also personally find definitions easier to read
when they are named explicitly. Here's an example that shows why:
typedef struct svn_foo_bar_t {
... 100 lines of member definitions ..
} svn_foo_bar_t;
(If it weren't for the explicit "svn_foo_bar_t" at the top of the
definition, I'd have to scan through 100 lines of member definitions
to find the name of the struct.)
Cheers,
David
--
David James -- http://www.cs.toronto.edu/~james
Re: svn commit: r16652 - trunk/subversion/include
Posted by Daniel Rall <dl...@finemaltcoding.com>.
On Tue, 11 Oct 2005, kfogel@collab.net wrote:
...
> Why is it not 'svn_opt_revision_value_t'? I thought we always used _t
> for types that are unaccompanied by some other type-indicating keyword
> (e.g., 'struct' or 'enum'). We should settle this now, since this is
> going into 1.3.
I too prefer the _t suffix, which is consistent with our struct naming
convention. While we're at it, why bother naming the enum when it's just
going to be typedef'd (e.g. does SWIG need that)? Leaving it anonymous
results in less code to read. My $0.02 USD.
- Dan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r16652 - trunk/subversion/include
Posted by kf...@collab.net.
Julian Foad <ju...@btopenworld.com> writes:
> If this has a public-style name because SWIG requires it, the comment
> should say so, otherwise a future programmer will likely change it to
> a private name.
I don't think the new type needs to be private. The point of the
comment is just to explain why things weren't done in the more
intuitive way, that is, with an inline anonymous type. No need to
give readers of the code an unnecessary scratch-head-and-think moment,
when a simple comment can explain what's going on.
I've done this in r16655.
Now for another question:
Why is it not 'svn_opt_revision_value_t'? I thought we always used _t
for types that are unaccompanied by some other type-indicating keyword
(e.g., 'struct' or 'enum'). We should settle this now, since this is
going into 1.3.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r16652 - trunk/subversion/include
Posted by Julian Foad <ju...@btopenworld.com>.
> djames@tigris.org writes:
>
>>--- trunk/subversion/include/svn_opt.h (original)
>>+++ trunk/subversion/include/svn_opt.h Tue Oct 11 10:01:32 2005
>>@@ -202,10 +202,12 @@
>> };
>>
>> /**
>>- * A revision value, which can be stored in one of @c svn_opt_revision_kind
>>- * ways.
>>+ * A revision value, which can be specified as a number or a date.
>>+ * @note This union should only be used in a @c svn_opt_revision_t struct.
Why? When I first saw this patch I thought "Do we really want to make this
symbol public? Probably not - but there's no harm in it. OK, let it be
public." Now your comment is effectively saying "This is private" - which is
fine except that I expect private things to have private names.
If this has a public-style name because SWIG requires it, the comment should
say so, otherwise a future programmer will likely change it to a private name.
On the other hand, if we use the union and need to make it public, why not let
other clients use it too?
- Julian
>>+ * @since New in 1.3.
>> */
>>-typedef union svn_opt_revision_value {
>>+typedef union svn_opt_revision_value
>>+{
>> svn_revnum_t number;
>> apr_time_t date;
>> } svn_opt_revision_value;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org