You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@apache.org> on 2018/12/30 23:07:23 UTC
Common header(s) for C and C++ API
Summary: I propose to create one or possibly several new public header
files that will be used by both the C and C++ public APIs.
I would like to completely hide the dependency on APR from the public
parts of the C++ API. In order to do that, public C++ headers must not
include the C headers directly, because most if not all of them do
expose APR.
Up to now I've been "solving" this in an unsatisfactory and error-prone
way, redefining enumeration values in C++ to be the same as in C,
forward-declaring C structures in C++ headers, and so on. Instead, I'd
like to extract certain parts of the C public API into a new header that
is independent of APR, and use that in both APIs. This way I won't avoid
redefining enumerations, for example, but I can ensure that both the C
and C++ APIs use the same enumeration constant values.
I'm attaching a patch that shows an example of what I have in mind.
Please raise your objections by next year. :)
-- Brane
Re: Common header(s) for C and C++ API
Posted by Branko Čibej <br...@apache.org>.
On Mon, 31 Dec 2018, 15:43 Julian Foad <julianfoad@apache.org wrote:
> I guess I'm missing something. Why not just move the existing C
> declarations verbatim to an APR-free header (from svn_types.h to
> svn_types_impl.h, for instance), and in C++ use them directly like this
> (proposed lines marked '>'):
>
> /**
> * @brief Revision number type.
> */
> - enum class number : long
> + enum class number : svn__impl__revnum_t
> > enum class number : svn_revnum_t
> {
> - invalid = -1, ///< Invalid revision number.
> + invalid = SVN__IMPL__INVALID_REVNUM, ///< Invalid revision number.
> > invalid = SVN_INVALID_REVNUM, ///< Invalid revision number.
> };
>
> Then there would be no need for all the reimplementing in svn_types.h with
> lines like "A = __impl__A".
>
That won't work, the type specifier on 'enum class' doesn't declare
inheritance but the underlying integral type of the enumeration constants.
The values of the constants still have to be defined explicitly. But yes, I
can skip one step as you suggest.
> Oh... I suppose the reason is because you don't want the C++ headers to
> expose the original C declarations.
Exposing the C declarations is not an issue, as long as it doesn't expose
APR types. I do want to use C++ 'enum class' for enumerations and e.g.,
svn_revnum_t, because they define distinct types which is important for
overload resolution and argument-dependent namespace lookup.
But is that such a problem? I'm thinking of the existing SWIG Python
> bindings where we map C name svn_client_add to Python name
> "svn.client.add", primarily, but we also leave the original name hanging
> around as "svn.client.svn_client_add" or something like that, and AFAIK
> it's not much of a problem, just a little untidy.
>
> Overall, if this is what it takes for you to proceed with the good work,
> then sure, go ahead: it looks like a manageable amount of churn.
>
I'll go with your suggestion to move the "safe" C types to separate headers
and use those directly in C++.
Thanks for the review!
-- Brane, not quite sure why he's answering mail on the phone on 1st
January, but surely wishing you all a not so bad New Year.
Re: Common header(s) for C and C++ API
Posted by Julian Foad <ju...@apache.org>.
I guess I'm missing something. Why not just move the existing C declarations verbatim to an APR-free header (from svn_types.h to svn_types_impl.h, for instance), and in C++ use them directly like this (proposed lines marked '>'):
/**
* @brief Revision number type.
*/
- enum class number : long
+ enum class number : svn__impl__revnum_t
> enum class number : svn_revnum_t
{
- invalid = -1, ///< Invalid revision number.
+ invalid = SVN__IMPL__INVALID_REVNUM, ///< Invalid revision number.
> invalid = SVN_INVALID_REVNUM, ///< Invalid revision number.
};
Then there would be no need for all the reimplementing in svn_types.h with lines like "A = __impl__A".
Oh... I suppose the reason is because you don't want the C++ headers to expose the original C declarations. But is that such a problem? I'm thinking of the existing SWIG Python bindings where we map C name svn_client_add to Python name "svn.client.add", primarily, but we also leave the original name hanging around as "svn.client.svn_client_add" or something like that, and AFAIK it's not much of a problem, just a little untidy.
Overall, if this is what it takes for you to proceed with the good work, then sure, go ahead: it looks like a manageable amount of churn.
--
- Julian
Re: Common header(s) for C and C++ API
Posted by Branko Čibej <br...@apache.org>.
On 31.12.2018 04:57, Troy Curtis Jr wrote:
> On Sun, Dec 30, 2018 at 6:07 PM Branko Čibej <br...@apache.org> wrote:
>
>> Summary: I propose to create one or possibly several new public header
>> files that will be used by both the C and C++ public APIs.
>>
>>
>> I would like to completely hide the dependency on APR from the public
>> parts of the C++ API. In order to do that, public C++ headers must not
>> include the C headers directly, because most if not all of them do
>> expose APR.
>>
>> Up to now I've been "solving" this in an unsatisfactory and error-prone
>> way, redefining enumeration values in C++ to be the same as in C,
>> forward-declaring C structures in C++ headers, and so on. Instead, I'd
>> like to extract certain parts of the C public API into a new header that
>> is independent of APR, and use that in both APIs. This way I won't avoid
>> redefining enumerations, for example, but I can ensure that both the C
>> and C++ APIs use the same enumeration constant values.
>>
> Just a few of the thoughts that occurred to me while thinking about this.
>
> How big do you expect these implementation header to get? In your included
> example it seems fairly reasonable, but I worry about what kinds of strange
> define/inclusion gymnastics might be required in the future in order to
> continue to accomplish the APR hiding goal.
I don't imagine these headers would be come very large. They can really
only contain plain data types, enunerations and forward declarations of
C structs (for p_impl, where apppropriate, so that we don't have to go
through void*).
> It also seems to me that in
> general you'd want to use module specific implementation files, like
> svn_client__impl.h or similar to main the nice componentized nature of the
> existing header files, which seems like quite a few files that would
> presumably be very empty. Would the "policy" be to put everything that
> doesn't require external headers into these implementation headers?
No, not everything; only things that can usefully be reused by the C++
API. I don't imagine there will be a lot of that. For example, I can't
imagine we'd ever expose the entire status callback structure in C++.
> Or only
> types/definitions that are also required for the CXX interface? Does that
> result in significant movement from the current headers to these new impl
> headers? Would it be a problem if it did? It seems like a good bit of
> churn, but probably not a very big deal.
My main concern is to not mess up the ABI. What I did in the example
with enumerations is safe in C, anything more complex would have to be
carefully checked and tested. The main reason for this move isn't so
much to save typing or the number of conformance tests, but to be able
to make some of the C++ stuff inline that currently isn't because it the
implementation relies on the C-API headers.
-- Brane
>> I'm attaching a patch that shows an example of what I have in mind.
>>
>> Please raise your objections by next year. :)
>>
>> -- Brane
>>
Re: Common header(s) for C and C++ API
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Sun, Dec 30, 2018 at 6:07 PM Branko Čibej <br...@apache.org> wrote:
> Summary: I propose to create one or possibly several new public header
> files that will be used by both the C and C++ public APIs.
>
>
> I would like to completely hide the dependency on APR from the public
> parts of the C++ API. In order to do that, public C++ headers must not
> include the C headers directly, because most if not all of them do
> expose APR.
>
> Up to now I've been "solving" this in an unsatisfactory and error-prone
> way, redefining enumeration values in C++ to be the same as in C,
> forward-declaring C structures in C++ headers, and so on. Instead, I'd
> like to extract certain parts of the C public API into a new header that
> is independent of APR, and use that in both APIs. This way I won't avoid
> redefining enumerations, for example, but I can ensure that both the C
> and C++ APIs use the same enumeration constant values.
>
Just a few of the thoughts that occurred to me while thinking about this.
How big do you expect these implementation header to get? In your included
example it seems fairly reasonable, but I worry about what kinds of strange
define/inclusion gymnastics might be required in the future in order to
continue to accomplish the APR hiding goal. It also seems to me that in
general you'd want to use module specific implementation files, like
svn_client__impl.h or similar to main the nice componentized nature of the
existing header files, which seems like quite a few files that would
presumably be very empty. Would the "policy" be to put everything that
doesn't require external headers into these implementation headers? Or only
types/definitions that are also required for the CXX interface? Does that
result in significant movement from the current headers to these new impl
headers? Would it be a problem if it did? It seems like a good bit of
churn, but probably not a very big deal.
> I'm attaching a patch that shows an example of what I have in mind.
>
> Please raise your objections by next year. :)
>
> -- Brane
>
Re: Common header(s) for C and C++ API
Posted by Paul Hammant <pa...@hammant.org>.
My bad - I assumed the contents of #1850344 were all new, but of
course the tests pre-existed that commit.
Re: Common header(s) for C and C++ API
Posted by Branko Čibej <br...@apache.org>.
On 04.01.2019 12:01, Paul Hammant wrote:
> http://svn.apache.org/viewvc?view=revision&revision=1850344
>
> ^ Any chance of example code being checked in too? Short
> how-to-embed-subversion for C++ and C, I mean. Others elsewhere could
> go on to do examples for Rust, Python, Java
> (https://github.com/bytedeco/javacpp), etc.
The only "use Subversion C API in C++" example code that I know of is
called JavaHL, and it's very much checked in.
If you mean examples for using the C++ API, that's far from finished so
it has no examples.
-- Brane
Re: Common header(s) for C and C++ API
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Paul Hammant wrote on Fri, 04 Jan 2019 11:01 +0000:
> ^ Any chance of example code being checked in too? Short
> how-to-embed-subversion for C++ and C, I mean.
There are some examples of using the C API in tools/. There
are also minimal examples in subversion/tests/cmdline/*.c .
Re: Common header(s) for C and C++ API
Posted by Paul Hammant <pa...@hammant.org>.
http://svn.apache.org/viewvc?view=revision&revision=1850344
^ Any chance of example code being checked in too? Short
how-to-embed-subversion for C++ and C, I mean. Others elsewhere could
go on to do examples for Rust, Python, Java
(https://github.com/bytedeco/javacpp), etc.
Re: Common header(s) for C and C++ API
Posted by Branko Čibej <br...@apache.org>.
On 31.12.2018 00:07, Branko Čibej wrote:
> Summary: I propose to create one or possibly several new public header
> files that will be used by both the C and C++ public APIs.
r1850344 is the result. Thanks again for the suggestions, I think this
is much cleaner than what I started with.
-- Brane
Re: Common header(s) for C and C++ API
Posted by Branko Čibej <br...@apache.org>.
On 01.01.2019 02:08, James McCoy wrote:
> On Mon, Dec 31, 2018 at 12:07:23AM +0100, Branko Čibej wrote:
>> Summary: I propose to create one or possibly several new public header
>> files that will be used by both the C and C++ public APIs.
>>
>>
>> I would like to completely hide the dependency on APR from the public
>> parts of the C++ API. In order to do that, public C++ headers must not
>> include the C headers directly, because most if not all of them do
>> expose APR.
>>
>> Up to now I've been "solving" this in an unsatisfactory and error-prone
>> way, redefining enumeration values in C++ to be the same as in C,
>> forward-declaring C structures in C++ headers, and so on. Instead, I'd
>> like to extract certain parts of the C public API into a new header that
>> is independent of APR, and use that in both APIs. This way I won't avoid
>> redefining enumerations, for example, but I can ensure that both the C
>> and C++ APIs use the same enumeration constant values.
>>
>> I'm attaching a patch that shows an example of what I have in mind.
>>
>> Please raise your objections by next year. :)
>>
>> -- Brane
>> Index: subversion/include/svn__impl__types.h
>> ===================================================================
>> --- subversion/include/svn__impl__types.h (nonexistent)
>> +++ subversion/include/svn__impl__types.h (working copy)
>> @@ -0,0 +1,95 @@
>> +/**
>> + * @copyright
>> + * ====================================================================
>> + * Licensed to the Apache Software Foundation (ASF) under one
>> + * or more contributor license agreements. See the NOTICE file
>> + * distributed with this work for additional information
>> + * regarding copyright ownership. The ASF licenses this file
>> + * to you under the Apache License, Version 2.0 (the
>> + * "License"); you may not use this file except in compliance
>> + * with the License. You may obtain a copy of the License at
>> + *
>> + * http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing,
>> + * software distributed under the License is distributed on an
>> + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
>> + * KIND, either express or implied. See the License for the
>> + * specific language governing permissions and limitations
>> + * under the License.
>> + * ====================================================================
>> + * @endcopyright
>> + *
>> + * @file svn_types.h
>> + * @brief Types implementation.
>> + * This is a @b private implementation-specific header file.
>> + * User code should not include it directly.
>> + */
>> +
>> +#ifndef SVN_X_IMPL_X_TYPES_H
>> +#define SVN_X_IMPL_X_TYPES_H
>> +
>> +/*
>> + * Define Subversion types that do not depend on APR or other
>> + * external libraries (except for the C standard libvrary) and are
>> + * shared betwen the C and C++ APIs. Do not include any headers
>> + * except for the C standard library headers here.
>> + */
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif /* __cplusplus */
>> +
>> +/* Tristate values
>> + * See: svn_tristate_t in svn_types.h
>> + * tristate in svnxx/tristate.hpp
>> + */
>> +enum svn__impl__tristate_t
> In C++, identifiers with a double underscore _anywhere_ are reserved.
> Although it's unlikely that these names will end up being used by a
> compiler implementation somewhere, we probably shouldn't be using them.
True, I'd forgotten about that. I'm too used to breaking that rule in
SVN's private/public APIs. :)
-- Brane
Re: Common header(s) for C and C++ API
Posted by James McCoy <ja...@jamessan.com>.
On Mon, Dec 31, 2018 at 12:07:23AM +0100, Branko Čibej wrote:
> Summary: I propose to create one or possibly several new public header
> files that will be used by both the C and C++ public APIs.
>
>
> I would like to completely hide the dependency on APR from the public
> parts of the C++ API. In order to do that, public C++ headers must not
> include the C headers directly, because most if not all of them do
> expose APR.
>
> Up to now I've been "solving" this in an unsatisfactory and error-prone
> way, redefining enumeration values in C++ to be the same as in C,
> forward-declaring C structures in C++ headers, and so on. Instead, I'd
> like to extract certain parts of the C public API into a new header that
> is independent of APR, and use that in both APIs. This way I won't avoid
> redefining enumerations, for example, but I can ensure that both the C
> and C++ APIs use the same enumeration constant values.
>
> I'm attaching a patch that shows an example of what I have in mind.
>
> Please raise your objections by next year. :)
>
> -- Brane
> Index: subversion/include/svn__impl__types.h
> ===================================================================
> --- subversion/include/svn__impl__types.h (nonexistent)
> +++ subversion/include/svn__impl__types.h (working copy)
> @@ -0,0 +1,95 @@
> +/**
> + * @copyright
> + * ====================================================================
> + * Licensed to the Apache Software Foundation (ASF) under one
> + * or more contributor license agreements. See the NOTICE file
> + * distributed with this work for additional information
> + * regarding copyright ownership. The ASF licenses this file
> + * to you under the Apache License, Version 2.0 (the
> + * "License"); you may not use this file except in compliance
> + * with the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing,
> + * software distributed under the License is distributed on an
> + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
> + * KIND, either express or implied. See the License for the
> + * specific language governing permissions and limitations
> + * under the License.
> + * ====================================================================
> + * @endcopyright
> + *
> + * @file svn_types.h
> + * @brief Types implementation.
> + * This is a @b private implementation-specific header file.
> + * User code should not include it directly.
> + */
> +
> +#ifndef SVN_X_IMPL_X_TYPES_H
> +#define SVN_X_IMPL_X_TYPES_H
> +
> +/*
> + * Define Subversion types that do not depend on APR or other
> + * external libraries (except for the C standard libvrary) and are
> + * shared betwen the C and C++ APIs. Do not include any headers
> + * except for the C standard library headers here.
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
> +/* Tristate values
> + * See: svn_tristate_t in svn_types.h
> + * tristate in svnxx/tristate.hpp
> + */
> +enum svn__impl__tristate_t
In C++, identifiers with a double underscore _anywhere_ are reserved.
Although it's unlikely that these names will end up being used by a
compiler implementation somewhere, we probably shouldn't be using them.
Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB