RE: Any idea why public function like svn_fspath__dirname have double __ in its name?
On Thu, 2010-12-30, Kamesh Jayachandran wrote: While we've mandated that __ must be used for semi-public functions, we've never said that it can't be used for private ones. Kamesh is talking about public, not private, functions. I.e., ones where we actually do have an ABI promise to keep. I looked at svn_error__locate last week. It's really only useful in --enable-maintainer-mode, but the way it's implemented, it ends up being used by macros in the public (non-maintainer) ABI, so even if we eliminate those callers, we have to supply the function itself forever. I've addressed this as best we can, in r1053469. Thanks Supplying a given ABI forever is the sort of thing I thought we didn't have to do for __ functions. I think that's Kamesh's question too. Yes. Following is my understanding, correct me if I am wrong. my understanding Any function/macro inside the header *directly* under subversion/include should only have a public functions(ones that starts with svn_ and do *not* have __). If somebody differs from this convention then there should be *subversion dev* community independent convention here may be like these. * doc string of such functions(semi-public a.k.a intra library functions/private(should we even have such one in headers?)) should promptly say so in the headers. * start with __svn like we see in some of the libc headers. The vague __ starting would signal about the scope/ABI nature of the API. /my understanding I added the svn_fspath__* functions. My understanding and my intention is: * The double underscore means this function is not for public use and does not have compatibility guarantees even though it is physically exported. * Functions that are not for public use should normally be in 'subversion/include/private/*.h'. These functions are in 'subversion/include/svn_dirent_uri.h' because they are analogous to other functions there, but we could move them. With regards Kamesh Jayachandran [PS] Unless I am mistaken svn_fspath__* can be used in libsvn_repos too instead of svn_path_* wherever applicable and hence a chance to become public. I don't quite understand what you mean here. - Julian
Re: Any idea why public function like svn_fspath__dirname have double __ in its name?
With regards Kamesh Jayachandran [PS] Unless I am mistaken svn_fspath__* can be used in libsvn_repos too instead of svn_path_* wherever applicable and hence a chance to become public. I don't quite understand what you mean here. I meant subversion/libsvn_repos/commit.c:delete_entry() uses deprecated svn_path_join which can be made to use svn_fspath__join. If my guess is true, we can as well make svn_fspath__join as svn_fspath_join and make it public as it is used by 2 modules(libsvn_fs and libsvn_repos). With regards Kamesh Jayachandran
Re: Any idea why public function like svn_fspath__dirname have double __ in its name?
On 01/05/2011 11:05 AM, Kamesh Jayachandran wrote: With regards Kamesh Jayachandran [PS] Unless I am mistaken svn_fspath__* can be used in libsvn_repos too instead of svn_path_* wherever applicable and hence a chance to become public. I don't quite understand what you mean here. I meant subversion/libsvn_repos/commit.c:delete_entry() uses deprecated svn_path_join which can be made to use svn_fspath__join. If my guess is true, we can as well make svn_fspath__join as svn_fspath_join and make it public as it is used by 2 modules(libsvn_fs and libsvn_repos). Our public API is no longer defined by what needs to be used by more than one module. It's defined by what functionality is required by or believed to be generally useful to third-party consumers of our libraries. Sometimes a function is used only inside a single module. We keep it module-private in that case. Sometimes a function is used by multiple modules, but isn't of any general utility to third-party consumers. We put it into include/private, so our own modules can use it, but it isn't part of our published public API (which has versioning promises and guarantees, etc.). (The svn_fspath__* functions might very well be examples of such functions.) And sometimes a function is used by one or more modules, but is clearly useful to third-party consumers of the Subversion libraries. Those functions live in include/ proper, and make up our public API. -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Distributed Development On Demand signature.asc Description: OpenPGP digital signature
Re: Any idea why public function like svn_fspath__dirname have double __ in its name?
On 28.12.2010 23:13, Peter Samuelson wrote: [C. Michael Pilato] svn_fspath__is_canonical svn_fspath__dirname svn_fspath__basename svn_fspath__split svn_fspath__join svn_fspath__is_child svn_fspath__skip_ancestor svn_fspath__is_ancestor svn_fspath__get_longest_ancestor svn_error__locate svn_error__malfunction While we've mandated that __ must be used for semi-public functions, we've never said that it can't be used for private ones. Kamesh is talking about public, not private, functions. I.e., ones where we actually do have an ABI promise to keep. I looked at svn_error__locate last week. It's really only useful in --enable-maintainer-mode, but the way it's implemented, it ends up being used by macros in the public (non-maintainer) ABI, so even if we eliminate those callers, we have to supply the function itself forever. I've addressed this as best we can, in r1053469. Now I'm really mystified. When I added those location-tracing functions and macros, they were only ever enabled with SVN_DEBUG turned on (i.e., in maintainer-mode). Now I see that those #ifdef wrappers are gone, and can't recall an explanation as to why that's a good thing. Can anyone remember what that change was good for? -- Brane
Re: Any idea why public function like svn_fspath__dirname have double __ in its name?
On 29.12.2010 09:43, Branko Čibej wrote: On 28.12.2010 23:13, Peter Samuelson wrote: [C. Michael Pilato] svn_fspath__is_canonical svn_fspath__dirname svn_fspath__basename svn_fspath__split svn_fspath__join svn_fspath__is_child svn_fspath__skip_ancestor svn_fspath__is_ancestor svn_fspath__get_longest_ancestor svn_error__locate svn_error__malfunction While we've mandated that __ must be used for semi-public functions, we've never said that it can't be used for private ones. Kamesh is talking about public, not private, functions. I.e., ones where we actually do have an ABI promise to keep. I looked at svn_error__locate last week. It's really only useful in --enable-maintainer-mode, but the way it's implemented, it ends up being used by macros in the public (non-maintainer) ABI, so even if we eliminate those callers, we have to supply the function itself forever. I've addressed this as best we can, in r1053469. Now I'm really mystified. When I added those location-tracing functions and macros, they were only ever enabled with SVN_DEBUG turned on (i.e., in maintainer-mode). Now I see that those #ifdef wrappers are gone, and can't recall an explanation as to why that's a good thing. Can anyone remember what that change was good for? Found it, r843793 and I agree with the change. So, effectively, svn_error__locate has been public since then, and r1053469 should be reverted anyway because it breaks the ABI. The name may or may not be misleading, depending on interpretation ... no-one should call svn_error__locate directly, the public API are effectively the macro wrappers. -- Brane
RE: Any idea why public function like svn_fspath__dirname have double __ in its name?
Now I'm really mystified. When I added those location-tracing functions and macros, they were only ever enabled with SVN_DEBUG turned on (i.e., in maintainer-mode). Now I see that those #ifdef wrappers are gone, and can't recall an explanation as to why that's a good thing. Can anyone remember what that change was good for? Found it, r843793 and I agree with the change. So, effectively, svn_error__locate has been public since then, and r1053469 should be reverted anyway because it breaks the ABI. Not sure why r1053469 need to be reverted. IIUC it still keeps the svn_error__locate by changing it to NOOP. Which I think is enough to keep ABI. With regards Kamesh Jayachandran
RE: Any idea why public function like svn_fspath__dirname have double __ in its name?
While we've mandated that __ must be used for semi-public functions, we've never said that it can't be used for private ones. Kamesh is talking about public, not private, functions. I.e., ones where we actually do have an ABI promise to keep. I looked at svn_error__locate last week. It's really only useful in --enable-maintainer-mode, but the way it's implemented, it ends up being used by macros in the public (non-maintainer) ABI, so even if we eliminate those callers, we have to supply the function itself forever. I've addressed this as best we can, in r1053469. Thanks Supplying a given ABI forever is the sort of thing I thought we didn't have to do for __ functions. I think that's Kamesh's question too. Yes. Following is my understanding, correct me if I am wrong. my understanding Any function/macro inside the header *directly* under subversion/include should only have a public functions(ones that starts with svn_ and do *not* have __). If somebody differs from this convention then there should be *subversion dev* community independent convention here may be like these. * doc string of such functions(semi-public a.k.a intra library functions/private(should we even have such one in headers?)) should promptly say so in the headers. * start with __svn like we see in some of the libc headers. The vague __ starting would signal about the scope/ABI nature of the API. /my understanding With regards Kamesh Jayachandran [PS] Unless I am mistaken svn_fspath__* can be used in libsvn_repos too instead of svn_path_* wherever applicable and hence a chance to become public.
Re: Any idea why public function like svn_fspath__dirname have double __ in its name?
[Branko Cibej] Found it, r843793 and I agree with the change. So, effectively, svn_error__locate has been public since then, and r1053469 should be reverted anyway because it breaks the ABI. It doesn't break the ABI. I kept the function itself. Whether it is useful to populate -file and -line, shipping potentially a huge number of constant strings and integers in our binaries that will never actually be seen, is the question. Essentially, we are reimplementing 'gcc -g' in a less useful, but impossible to omit or strip out, way. Impossible to omit or strip out is, I guess, the selling point. But I think I'd rather just see '-g' added to CFLAGS by default. The name may or may not be misleading, depending on interpretation ... no-one should call svn_error__locate directly, the public API are effectively the macro wrappers. Fine, svn_error__locate is not part of the public API, but it is very much part of the ABI. I think it's annoying that we have this __ function that we're not allowed to remove, or change the calling signature. Certainly it's not something we can fix _now_, but we should definitely avoid doing more of this in the future. How I noticed: in the Debian builds, I ship a semi-autogenerated list of public symbols and the version of libsvn in which they first appeared. This is for automatic dependency generation when building other packages; most libraries with backward-compatible ABIs in Debian do this, and it works very well. In that list I grep out all the svn_*__ symbols, as nobody should be using those. Thus we discovered that certain symbols like svn_error__locate do appear in binaries, through no fault of the third-party developers. -- Peter Samuelson | org-tld!p12n!peter | http://p12n.org/
Re: Any idea why public function like svn_fspath__dirname have double __ in its name?
On 29.12.2010 20:10, Kamesh Jayachandran wrote: Now I'm really mystified. When I added those location-tracing functions and macros, they were only ever enabled with SVN_DEBUG turned on (i.e., in maintainer-mode). Now I see that those #ifdef wrappers are gone, and can't recall an explanation as to why that's a good thing. Can anyone remember what that change was good for? Found it, r843793 and I agree with the change. So, effectively, svn_error__locate has been public since then, and r1053469 should be reverted anyway because it breaks the ABI. Not sure why r1053469 need to be reverted. IIUC it still keeps the svn_error__locate by changing it to NOOP. Which I think is enough to keep ABI. Read the log of r843793 to see why -- the idea is not to make it a no-op, but to keep the location info there for debuggers. Granted, that could be done in a better way, however, the macros need to stay outside of SVN_DEBUG until someone implements that better way. -- Brane
Re: Any idea why public function like svn_fspath__dirname have double __ in its name?
On 29.12.2010 20:50, Peter Samuelson wrote: [Branko Cibej] Found it, r843793 and I agree with the change. So, effectively, svn_error__locate has been public since then, and r1053469 should be reverted anyway because it breaks the ABI. It doesn't break the ABI. I kept the function itself. Sorry, I missed that in the diff. Whether it is useful to populate -file and -line, shipping potentially a huge number of constant strings and integers in our binaries that will never actually be seen, is the question. Essentially, we are reimplementing 'gcc -g' in a less useful, but impossible to omit or strip out, way. Impossible to omit or strip out is, I guess, the selling point. But I think I'd rather just see '-g' added to CFLAGS by default. -g isn't exactly the same thing, certainly not on compilers that cannot generate optimized code whilst generating useful debug info. I see no reason to put those macros inside an ifdef at this point. The name may or may not be misleading, depending on interpretation ... no-one should call svn_error__locate directly, the public API are effectively the macro wrappers. Fine, svn_error__locate is not part of the public API, but it is very much part of the ABI. I think it's annoying that we have this __ function that we're not allowed to remove, or change the calling signature. Certainly it's not something we can fix _now_, but we should definitely avoid doing more of this in the future. How I noticed: in the Debian builds, I ship a semi-autogenerated list of public symbols and the version of libsvn in which they first appeared. This is for automatic dependency generation when building other packages; most libraries with backward-compatible ABIs in Debian do this, and it works very well. In that list I grep out all the svn_*__ symbols, as nobody should be using those. Thus we discovered that certain symbols like svn_error__locate do appear in binaries, through no fault of the third-party developers We're allowed to make exceptions. This particular exception has been in the code since before 1.0 was released (and before our versioning and naming and API revving rules were formalized). There's really no good reason to go changing it now. So we have an inconsistency in our A[PB]I. To which I respond, Emerson! -- Brane
Any idea why public function like svn_fspath__dirname have double __ in its name?
Hi All, I could see the following functions in our public header with '__' in their names, svn_fspath__is_canonical svn_fspath__dirname svn_fspath__basename svn_fspath__split svn_fspath__join svn_fspath__is_child svn_fspath__skip_ancestor svn_fspath__is_ancestor svn_fspath__get_longest_ancestor svn_error__locate svn_error__malfunction IIUC __ convention is used for semi-public functions where we may not keep BC promise. Do I miss something? With regards Kamesh Jayachandran
Re: Any idea why public function like svn_fspath__dirname have double __ in its name?
On 12/28/2010 04:30 AM, Kamesh Jayachandran wrote: Hi All, I could see the following functions in our public header with '__' in their names, svn_fspath__is_canonical svn_fspath__dirname svn_fspath__basename svn_fspath__split svn_fspath__join svn_fspath__is_child svn_fspath__skip_ancestor svn_fspath__is_ancestor svn_fspath__get_longest_ancestor svn_error__locate svn_error__malfunction IIUC __ convention is used for semi-public functions where we may not keep BC promise. Do I miss something? While we've mandated that __ must be used for semi-public functions, we've never said that it can't be used for private ones. One could argue that the use of __ for even private functions avoids a required rename if the function is made semi-public, and adds information. For example, I know that most of the functions you list above are FS-related and probably logically related. But I wouldn't suggest a universal policy change at this stage. -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Distributed Development On Demand signature.asc Description: OpenPGP digital signature
Re: Any idea why public function like svn_fspath__dirname have double __ in its name?
[C. Michael Pilato] svn_fspath__is_canonical svn_fspath__dirname svn_fspath__basename svn_fspath__split svn_fspath__join svn_fspath__is_child svn_fspath__skip_ancestor svn_fspath__is_ancestor svn_fspath__get_longest_ancestor svn_error__locate svn_error__malfunction While we've mandated that __ must be used for semi-public functions, we've never said that it can't be used for private ones. Kamesh is talking about public, not private, functions. I.e., ones where we actually do have an ABI promise to keep. I looked at svn_error__locate last week. It's really only useful in --enable-maintainer-mode, but the way it's implemented, it ends up being used by macros in the public (non-maintainer) ABI, so even if we eliminate those callers, we have to supply the function itself forever. I've addressed this as best we can, in r1053469. Supplying a given ABI forever is the sort of thing I thought we didn't have to do for __ functions. I think that's Kamesh's question too. -- Peter Samuelson | org-tld!p12n!peter | http://p12n.org/
