You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "C. Michael Pilato" <cm...@collab.net> on 2004/09/20 22:28:45 UTC

svn_repos_trace_node_locations()

Would it violate our API promises to add 'const' to the
location_revisions parameter of this function in svn_repos.h:

   svn_error_t *
   svn_repos_trace_node_locations (svn_fs_t *fs,
                                   apr_hash_t **locations,
                                   const char *fs_path,
                                   svn_revnum_t peg_revision,
                                   apr_array_header_t *location_revisions,
                                   apr_pool_t *pool);

?

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn_repos_trace_node_locations()

Posted by Philip Martin <ph...@codematters.co.uk>.
"C. Michael Pilato" <cm...@collab.net> writes:

> Would it violate our API promises to add 'const' to the
> location_revisions parameter of this function in svn_repos.h:
>
>    svn_error_t *
>    svn_repos_trace_node_locations (svn_fs_t *fs,
>                                    apr_hash_t **locations,
>                                    const char *fs_path,
>                                    svn_revnum_t peg_revision,
>                                    apr_array_header_t *location_revisions,
>                                    apr_pool_t *pool);
>
> ?

I believe it is ABI compatible on a gcc/Linux platform.  As far as I
can recall, the same applied on the various Unix platforms I have
used in the past.

Looking at the C standard

 6.5.2.2/9   Function calls

    "If the function is defined with a type that is not compatible
    with the type (of the expression) pointed to by the expression
    that denotes the called function, the behaviour is undefined."

 6.7.5.3/15  Function declarators (including prototypes)

    "For two function types to be compatible [...] the parameter type
    lists [...] shall agree in the number of parameters [...] ;
    corresponding parameters shall have compatible types"

 6.7.3/9     Type qualifiers

    "For two qualifed types to be compatible, both shall have the
    identically qualified version of a compatible type"

So it would appear to be undefined behaviour--in other words, it's
"nasal demons" territory.

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn_repos_trace_node_locations()

Posted by Philip Martin <ph...@codematters.co.uk>.
Greg Hudson <gh...@MIT.EDU> writes:

> I think it would be just fine to add "const" to the parameter, in a
> 1.x.0 release.  All it does is make the function a little nicer.  Issue
> #1952 is different because it's changing the expected signature of a
> callback ("your callback must be a little nicer").
>
> I know Philip raised some ABI concerns based on the C standard, but
> really, the C standard has little bearing on how API translates into
> ABI; that's up to the implementations.  And implementations don't care
> what you declare const when determining ABIs.

If this were a C++ function then adding const would definitely break
the ABI.  While I've never come across a C implementation that works
in the same way, I suppose it could be done.  In my earlier message I
was careful to state that I didn't think there would be any ABI
problems on Linux, or the other Unix platforms I have used, and I
certainly won't object if the change is made.

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn_repos_trace_node_locations()

Posted by Greg Hudson <gh...@MIT.EDU>.
On Fri, 2004-10-01 at 12:11, Archie Cobbs wrote:
> Julian Foad wrote:
> > >> Would it violate our API promises to add 'const' to the
> > >> location_revisions parameter of this function in svn_repos.h:
> > >>
> > >>   svn_error_t *
> > >>   svn_repos_trace_node_locations (svn_fs_t *fs,
> > >>                                   apr_hash_t **locations,
> > >>                                   const char *fs_path,
> > >>                                   svn_revnum_t peg_revision,
> > >>                                   apr_array_header_t *location_revisions,
> > >>                                   apr_pool_t *pool);

> I don't see how adding "const" would break the API.

You're right.  I followed this thread for a while but initially had the
wrong conclusion, so I didn't speak up because I thought CMike had
gotten the right answer.

I think it would be just fine to add "const" to the parameter, in a
1.x.0 release.  All it does is make the function a little nicer.  Issue
#1952 is different because it's changing the expected signature of a
callback ("your callback must be a little nicer").

I know Philip raised some ABI concerns based on the C standard, but
really, the C standard has little bearing on how API translates into
ABI; that's up to the implementations.  And implementations don't care
what you declare const when determining ABIs.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn_repos_trace_node_locations()

Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Fri, 1 Oct 2004, Archie Cobbs wrote:

> Julian Foad wrote:
> > >> Would it violate our API promises to add 'const' to the
> > >> location_revisions parameter of this function in svn_repos.h:
> > >>
> > >>   svn_error_t *
> > >>   svn_repos_trace_node_locations (svn_fs_t *fs,
> > >>                                   apr_hash_t **locations,
> > >>                                   const char *fs_path,
> > >>                                   svn_revnum_t peg_revision,
> > >>                                   apr_array_header_t *location_revisions,
> > >>                                   apr_pool_t *pool);
> > >>
> > >> ?
> > >
> > > I'm guessing 'yes', since we already have this issue:
> > > http://subversion.tigris.org/issues/show_bug.cgi?id=1952
> > > which is purely a reminder to const-ify a different pointer come 2.0.
> >
> > May I suggest you add this to issue #1952, and broaden the summary of the issue to cover all such 'const' additions?
>
> I don't see how adding "const" would break the API. That change
> would simply acknowledge to new application code what is already
> true in the library code, namely, that svn_repos_trace_node_locations()
> does not modify *location_revisions. Maybe I don't understand the
> "API promises".
>
I think someone quoted the C standard where it says that if you call a
function and it doesn't have exactly the same signature as your
declaration (including const qualification), behaviour is undefined.
Remember that this is about binary compability. Feel free to just correct
me if I'm wrong...

Regards,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn_repos_trace_node_locations()

Posted by Archie Cobbs <ar...@dellroad.org>.
Julian Foad wrote:
> >> Would it violate our API promises to add 'const' to the
> >> location_revisions parameter of this function in svn_repos.h:
> >>
> >>   svn_error_t *
> >>   svn_repos_trace_node_locations (svn_fs_t *fs,
> >>                                   apr_hash_t **locations,
> >>                                   const char *fs_path,
> >>                                   svn_revnum_t peg_revision,
> >>                                   apr_array_header_t *location_revisions,
> >>                                   apr_pool_t *pool);
> >>
> >> ?
> > 
> > I'm guessing 'yes', since we already have this issue:
> > http://subversion.tigris.org/issues/show_bug.cgi?id=1952
> > which is purely a reminder to const-ify a different pointer come 2.0.
> 
> May I suggest you add this to issue #1952, and broaden the summary of the issue to cover all such 'const' additions?

I don't see how adding "const" would break the API. That change
would simply acknowledge to new application code what is already
true in the library code, namely, that svn_repos_trace_node_locations()
does not modify *location_revisions. Maybe I don't understand the
"API promises".

-Archie

__________________________________________________________________________
Archie Cobbs      *        CTO, Awarix        *      http://www.awarix.com

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn_repos_trace_node_locations()

Posted by Julian Foad <ju...@btopenworld.com>.
Max Bowsher wrote:
> C. Michael Pilato wrote:
> 
>> Would it violate our API promises to add 'const' to the
>> location_revisions parameter of this function in svn_repos.h:
>>
>>   svn_error_t *
>>   svn_repos_trace_node_locations (svn_fs_t *fs,
>>                                   apr_hash_t **locations,
>>                                   const char *fs_path,
>>                                   svn_revnum_t peg_revision,
>>                                   apr_array_header_t *location_revisions,
>>                                   apr_pool_t *pool);
>>
>> ?
> 
> I'm guessing 'yes', since we already have this issue:
> http://subversion.tigris.org/issues/show_bug.cgi?id=1952
> which is purely a reminder to const-ify a different pointer come 2.0.

May I suggest you add this to issue #1952, and broaden the summary of the issue to cover all such 'const' additions?

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn_repos_trace_node_locations()

Posted by Max Bowsher <ma...@ukf.net>.
C. Michael Pilato wrote:
> Would it violate our API promises to add 'const' to the
> location_revisions parameter of this function in svn_repos.h:
> 
>   svn_error_t *
>   svn_repos_trace_node_locations (svn_fs_t *fs,
>                                   apr_hash_t **locations,
>                                   const char *fs_path,
>                                   svn_revnum_t peg_revision,
>                                   apr_array_header_t *location_revisions,
>                                   apr_pool_t *pool);
> 
> ?

I'm guessing 'yes', since we already have this issue:
http://subversion.tigris.org/issues/show_bug.cgi?id=1952
which is purely a reminder to const-ify a different pointer come 2.0.

I can't find the associated mail threads, though.

Max.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org