[PATCH] D53936: [clang-tidy] More clearly separate public, check-facing APIs from internal ones.

[Stephen Kelly via Phabricator via cfe-commits]

steveire added inline comments.


================
Comment at: clang-tidy/ClangTidy.h:11
+//
+// It should remain as stable as possible, as many out-of-tree checks exist.
+//===----------------------------------------------------------------------===//
----------------
alexfh wrote:
> sammccall wrote:
> > steveire wrote:
> > > Clang C++ code does not have any stability requirements. That's quite well-established.
> > > 
> > > This comment adds a new requirement of stability for this C++ API. You should probably put a RFC on cfe-dev about it. This header is no more public or stable than any other Clang header.
> > > 
> > > 
> > > Clang C++ code does not have any stability requirements. That's quite well-established.
> > 
> > I was aiming for "stability is a design goal", not "this is a stable API and you may not break it".
> > Happy to take a shot at rewording, maybe you have suggestions?
> > 
> > > This comment adds a new requirement of stability for this C++ API. You should probably put a RFC on cfe-dev about it.
> > 
> > Stability here has always been an aim; I'm trying to document the current state of the world.
> > 
> > e.g. in D15528 @alexfh wrote:
> > > It's one of the goals of clang-tidy to provide an easy way to maintain out-of-tree checks.
> > 
> > > This header is no more public or stable than any other Clang header.
> > I wish that were true - my current yakshave is reducing the need for `registerPPCallbacks` to make clang-tidy more flexible for use in a library. Removing it entirely would be great but having talked to some clang-tidy maintainers, out-of-tree checks are at least a factor here.
> I tend to agree with Stephen re: lack of stability guarantees of Clang APIs. Clang-tidy API on its own is not nearly enough to create a useful check, thus any out-of-tree check will use a significant amount of Clang APIs. Though the project aims at providing an easy way to maintain out-of-tree checks, there's little benefit in clang-tidy APIs being more stable than the API surface of Clang needed for an average out-of-tree check. Maintainers of any clang-based out-of-tree code should be ready to update their code with every change of the clang revision they use. Clang-tidy checks are not an exception. Putting this documentation here may send a very wrong message and serve badly both to the clang-tidy developers and to the folks who decide to depend on it.
> 
> A softer version of this (something along the lines of "A large number of out-of-tree checks depend on this API, so be careful when introducing potentially breaking changes.") may work here, but the same note will be needed on the whole AST, ASTMatchers, and Lex libraries in Clang (and many other ones, I guess). 
I have a proposal coming up which involves a breaking change to AST_MATCHER* macros (many of which are out of tree), so I'm really just trying to make sure no backward compatibility concern opposes that.

I still see no point in the comment, if you don't also put the same comment in ASTMatchers.h, each header in AST/ etc. The files have the same out-of-tree use, and no one of them deserves a comment more than the others.

In fact, I had the idea just last week that I want to write a documentation page outlining compatibility guarantees in various user-facing parts of llvm/clang. It would be a single page containing the two sections about compatibility here: https://llvm.org/docs/DeveloperPolicy.html#ir-backwards-compatibility, C++ API stability, whether clang-query interpreter commands are stable, whether driver command lines and plumbing command lines are stable etc. AFAIK, that does not exist yet.

Regarding C++ API compatibility something along the lines here (http://clang-developers.42468.n3.nabble.com/getLocStart-versus-getStartLoc-tp4061010p4061292.html) and here (http://clang-developers.42468.n3.nabble.com/API-Removal-Notice-4-weeks-getStartLoc-getLocStart-getLocEnd-td4061566.html) . A note like yours would belong on such a page too. I don't think it belongs in this header.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53936