On Mon, Dec 29, 2014 at 4:59 PM, Branko Čibej <br...@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".
