On Mon, Dec 29, 2014 at 4:59 PM, Branko Čibej <brane_at_wandisco.com> wrote:
> On 29.12.2014 13:59, Stefan Fuhrmann wrote:
> > Hi there,
> >
> > FSX code contains various violations to our naming rules,
> > mostly taken over FSFS. I thought about a scheme that
> > complies to our rules but also refines them.
> >
> > I'd like to amend our coding guideline with the following
> > suggestions (not as a strict requirement). The first one is
> > actually "new" while the other two have already been used
> > by various portions of our fs_* libs:
> >
> > 1. Use the svn_ prefix only for identifiers meant to be used
> > by other libs, i.e. only for declarations in the ./include sub-
> > tree. We currently lack that distinction and it lead to minor
> > confusion in the past.
>
> If it's a non-static identifier (i.e., if it can possibly be exported
> from some library), then we have to retain the svn_ prefix in order to
> reduce the chance of name collisions in downstream uses.
>
Well, that blows it out of the water basically ... :/
Maybe in our coding rules, we should state explicitly that all
non-static identifiers in libraries need an svn_ / SVN_ prefix.
> 2. If the library name contains multiple elements, use only
> > the last one as prefix for internal identifiers. In combination
> > with the svn_ prefix, use the full library name.
> >
> > 3. Static functions implementing a v-table function should be
> > named <lib>_<func>. <func> is the name of the respective
> > API function without its prefixes.
> >
> > Combined with the existing rules, this is how a "commit"
> > function in libsvn_fs_base would be named. This is a theoretical
> > example; not all of these identifiers actually do exist in SVN:
> >
> > * commit - static function
> > * base_commit - static function implementing svn_fs_commit
>
> This is almost what we're already doing, although I have trouble
> visualising a connection between "base" and "svn_fs". It would be far
> more obvious to use the name "fs_base_commit", which brings us back to
> our current naming convention.
>
> I suspect you're trying invent a way to use shorter identifiers. While
> that's a commendable goal, it's never a good idea to do that at the
> expense of clarity.
>
Currently, the code says "base_commit" and is probably
even inconsistent about that. I borrowed the idea to keep
identifiers short. Imagine, 3 of them would fit into 1 LOC ;)
If there was such a thing as library-local identifiers, any
prefix before the "__" would do just fine - it could be only
from the current lib. Since lib-only-scope does not exist,
always using the full lib prefix (but without svn_) is probably
nicer. They are only referenced by vtables, so there is
no convenience problem.
> > * base__commit - library-local, defined in some local header
>
> There's no such thing as library-local symbols; see above. In general,
> you cannot expect to be able to hide such symbols from external library
> users. Yes, there are tricks you can use in some object formats, but we
> don't rely on them because they're rarely portable (in the semantic
> sense) between formats and platforms.
>
> I'd be overjoyed if someone could demonstrate that we can create truly
> library-local symbol names that cannot be accidentally exported, and
> that we can consistently do this on all currently supported and future
> platforms. However, I suspect this may be a futile goal.
>
The two theoretical options I see are not to support static
linkage of SVN with non-SVN libs or to compile the libs
as C++ with explicit C exports. Option 1 is a complete
no-go for many reasons. Option 2 is at least plausible but
still to much effort for solving a convenience issue.
> > * svn_fs_base__commit - to be used by any SVN lib and must
> > be declared in ./include/private
> > * svn_fs_base_commit - to be used by any part of an application,
> > subject to compatibility rules and must be declared in ./include
>
> IIUC another thing you're trying to solve is the ... call it
> "ideological" difference between private symbols declared in
> ./include/private and such symbols declared in library-local headers.
>
Yes, it has been about expressing the mental model
of "private" and "public" wrt. to library boundaries.
I remember Evgeny asking me about some "weird"
internal function which was a mere technical artefact
of having caller and callee in different compilation units.
> Our current naming convention for both is svn_libname__symbol, which I
> agree may be confusing. But ... as I said, I do not agree that we can
> safely drop the svn_ prefix from so-called "library-local" symbols.
>
> An obvious alternative would be
>
> svn__fs_base_commit
>
> vs.
>
> svn_fs_base__commit
>
> but that's even more confusing; as is svn__fs_base__commit. I suspect
> the best solution for this case would be to invent a completely new
> prefix; consider, e.g., xvn__fs_base_commit; which is different from the
> current private API naming convention.
>
Or svn_local_fs_base__ ...
> I'm not sure what to do here, especially as our "library-private" naming
> is already less than consistent. Inventing a new convention for these
> names implies that, at some point, we'd have to rename them all ...
> which would involve a lot of code churn for dubious benefit.
>
The sole purpose of me post was to ask for feedback
before getting on a rename spree in FSX,which is still
necessary mainly due to structs not being named properly.
If I could, I wanted to get rid of the idiosyncrasies of
typing "svn_fs_x__func" where "x__func" would do.
However, I had missed the limitations of the C linker.
Now there are only two points left
* Be explicit in our coding style guide about the mandatory
svn_ prefix in all non-static identifiers.
* Don't shorten library names. No need to fix that for older
FS but get it right in FSX.
-- Stefan^2.
P.S.: I just discovered that my IDE will suggest
"svn_fs_x__func" as soon as I type "x__fu".
Received on 2015-01-02 13:33:37 CET