Re: svn commit: r1850344 - in /subversion/trunk/subversion: bindings/cxx/include/svnxx/ bindings/cxx/src/ include/

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1850344 - in /subversion/trunk/subversion: bindings/cxx/include/svnxx/ bindings/cxx/src/ include/

Daniel Shahaf-2
[hidden email] wrote on Fri, 04 Jan 2019 10:38 +0000:
> Move (some of the) standalone types into separate implementation headers
> so that SVN++ can use them directly without exposing APR or other dependencies.

> +++ subversion/trunk/subversion/include/svn_opt_impl.h Fri Jan  4
> 10:38:53 2019
> @@ -0,0 +1,83 @@
> + * @file svn_opt.h
> + * @brief Option and argument parsing for Subversion command lines
> + *        (common implementation)
> + */
> +
> +/* NOTE:
> + * This file *must not* include or depend on any other header except
> + * the C standard library headers.
> + */

Could the comment also explain the rationale for the restriction it imposes?

> +
> +#ifndef SVN_OPT_IMPL_H
> +#define SVN_OPT_IMPL_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1850344 - in /subversion/trunk/subversion: bindings/cxx/include/svnxx/ bindings/cxx/src/ include/

Branko Čibej
On 04.01.2019 15:20, Daniel Shahaf wrote:

> [hidden email] wrote on Fri, 04 Jan 2019 10:38 +0000:
>> Move (some of the) standalone types into separate implementation headers
>> so that SVN++ can use them directly without exposing APR or other dependencies.
>> +++ subversion/trunk/subversion/include/svn_opt_impl.h Fri Jan  4
>> 10:38:53 2019
>> @@ -0,0 +1,83 @@
>> + * @file svn_opt.h
>> + * @brief Option and argument parsing for Subversion command lines
>> + *        (common implementation)
>> + */
>> +
>> +/* NOTE:
>> + * This file *must not* include or depend on any other header except
>> + * the C standard library headers.
>> + */
> Could the comment also explain the rationale for the restriction it imposes?

No, not in every "impl" header we happen to create. But it is documented
in subversion/bindings/cxx/README, as one of the SVN++ design goals.

-- Brane

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1850344 - in /subversion/trunk/subversion: bindings/cxx/include/svnxx/ bindings/cxx/src/ include/

Daniel Shahaf-2
Branko Čibej wrote on Fri, 04 Jan 2019 15:31 +0100:

> On 04.01.2019 15:20, Daniel Shahaf wrote:
> > [hidden email] wrote on Fri, 04 Jan 2019 10:38 +0000:
> >> Move (some of the) standalone types into separate implementation headers
> >> so that SVN++ can use them directly without exposing APR or other dependencies.
> >> +++ subversion/trunk/subversion/include/svn_opt_impl.h Fri Jan  4
> >> 10:38:53 2019
> >> @@ -0,0 +1,83 @@
> >> + * @file svn_opt.h
> >> + * @brief Option and argument parsing for Subversion command lines
> >> + *        (common implementation)
> >> + */
> >> +
> >> +/* NOTE:
> >> + * This file *must not* include or depend on any other header except
> >> + * the C standard library headers.
> >> + */
> > Could the comment also explain the rationale for the restriction it imposes?
>
> No, not in every "impl" header we happen to create. But it is documented
> in subversion/bindings/cxx/README, as one of the SVN++ design goals.

Shouldn't this be documented in the C API's documentation too, at least
by reference?  Someone working on the C headers in a year or three might
not think to look in the C++ bindings for design choices of the C API.
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1850344 - in /subversion/trunk/subversion: bindings/cxx/include/svnxx/ bindings/cxx/src/ include/

Branko Čibej
On 04.01.2019 15:35, Daniel Shahaf wrote:

> Branko Čibej wrote on Fri, 04 Jan 2019 15:31 +0100:
>> On 04.01.2019 15:20, Daniel Shahaf wrote:
>>> [hidden email] wrote on Fri, 04 Jan 2019 10:38 +0000:
>>>> Move (some of the) standalone types into separate implementation headers
>>>> so that SVN++ can use them directly without exposing APR or other dependencies.
>>>> +++ subversion/trunk/subversion/include/svn_opt_impl.h Fri Jan  4
>>>> 10:38:53 2019
>>>> @@ -0,0 +1,83 @@
>>>> + * @file svn_opt.h
>>>> + * @brief Option and argument parsing for Subversion command lines
>>>> + *        (common implementation)
>>>> + */
>>>> +
>>>> +/* NOTE:
>>>> + * This file *must not* include or depend on any other header except
>>>> + * the C standard library headers.
>>>> + */
>>> Could the comment also explain the rationale for the restriction it imposes?
>> No, not in every "impl" header we happen to create. But it is documented
>> in subversion/bindings/cxx/README, as one of the SVN++ design goals.
> Shouldn't this be documented in the C API's documentation too, at least
> by reference?  Someone working on the C headers in a year or three might
> not think to look in the C++ bindings for design choices of the C API.

If you have an idea how to do that without boring repetition, please go
ahead.

I just don't see it as all that relevant apart from the "don't do that"
header in the files. Every existing type that was moved to the new
headers already has a reference to its C++ counterpart[1]. When we
invent new types, trivial cases will be caught when (if?) their C++
wrappers are written.

-- Brane

[1] ... except svn_node_kind_t, which doesn't have a C++ counterpart yet.

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1850344 - in /subversion/trunk/subversion: bindings/cxx/include/svnxx/ bindings/cxx/src/ include/

Daniel Shahaf-2
Branko Čibej wrote on Fri, 04 Jan 2019 15:42 +0100:

> On 04.01.2019 15:35, Daniel Shahaf wrote:
> > Branko Čibej wrote on Fri, 04 Jan 2019 15:31 +0100:
> >> On 04.01.2019 15:20, Daniel Shahaf wrote:
> >>> [hidden email] wrote on Fri, 04 Jan 2019 10:38 +0000:
> >>>> Move (some of the) standalone types into separate implementation headers
> >>>> so that SVN++ can use them directly without exposing APR or other dependencies.
> >>>> +++ subversion/trunk/subversion/include/svn_opt_impl.h Fri Jan  4
> >>>> 10:38:53 2019
> >>>> @@ -0,0 +1,83 @@
> >>>> + * @file svn_opt.h
> >>>> + * @brief Option and argument parsing for Subversion command lines
> >>>> + *        (common implementation)
> >>>> + */
> >>>> +
> >>>> +/* NOTE:
> >>>> + * This file *must not* include or depend on any other header except
> >>>> + * the C standard library headers.
> >>>> + */
> >>> Could the comment also explain the rationale for the restriction it imposes?
> >> No, not in every "impl" header we happen to create. But it is documented
> >> in subversion/bindings/cxx/README, as one of the SVN++ design goals.
> > Shouldn't this be documented in the C API's documentation too, at least
> > by reference?  Someone working on the C headers in a year or three might
> > not think to look in the C++ bindings for design choices of the C API.
>
> If you have an idea how to do that without boring repetition, please go
> ahead.

We could add a paragraph to HACKING, or possibly to a single global
doxygen file, explaining the _impl.h convention, rather than repeating
the explanation in each individual file.
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1850344 - in /subversion/trunk/subversion: bindings/cxx/include/svnxx/ bindings/cxx/src/ include/

Branko Čibej
On 04.01.2019 15:48, Daniel Shahaf wrote:

> Branko Čibej wrote on Fri, 04 Jan 2019 15:42 +0100:
>> On 04.01.2019 15:35, Daniel Shahaf wrote:
>>> Branko Čibej wrote on Fri, 04 Jan 2019 15:31 +0100:
>>>> On 04.01.2019 15:20, Daniel Shahaf wrote:
>>>>> [hidden email] wrote on Fri, 04 Jan 2019 10:38 +0000:
>>>>>> Move (some of the) standalone types into separate implementation headers
>>>>>> so that SVN++ can use them directly without exposing APR or other dependencies.
>>>>>> +++ subversion/trunk/subversion/include/svn_opt_impl.h Fri Jan  4
>>>>>> 10:38:53 2019
>>>>>> @@ -0,0 +1,83 @@
>>>>>> + * @file svn_opt.h
>>>>>> + * @brief Option and argument parsing for Subversion command lines
>>>>>> + *        (common implementation)
>>>>>> + */
>>>>>> +
>>>>>> +/* NOTE:
>>>>>> + * This file *must not* include or depend on any other header except
>>>>>> + * the C standard library headers.
>>>>>> + */
>>>>> Could the comment also explain the rationale for the restriction it imposes?
>>>> No, not in every "impl" header we happen to create. But it is documented
>>>> in subversion/bindings/cxx/README, as one of the SVN++ design goals.
>>> Shouldn't this be documented in the C API's documentation too, at least
>>> by reference?  Someone working on the C headers in a year or three might
>>> not think to look in the C++ bindings for design choices of the C API.
>> If you have an idea how to do that without boring repetition, please go
>> ahead.
> We could add a paragraph to HACKING, or possibly to a single global
> doxygen file, explaining the _impl.h convention, rather than repeating
> the explanation in each individual file.

A Doxygen file wouldn't work since we don't use generated docs for
developer guidelines, but a paragraph in HACKING about C++ bindings and
similar policy would work.

-- Brane