[PATCH] Initial clang-tidy architecture

[Manuel Klimek]

On Tue, Jun 4, 2013 at 10:12 PM, Doug Gregor <dgregor at apple.com> wrote:

>
>   The general architecture looks good here. A few comments:
>
>   1) I agree with Chandler that we want to avoiding adding another
> diagnostic mechanism to Clang. There's a lot of extra baggage that each
> diagnostic mechanism brings, and I'd much rather we extend/refactor Clang's
> primary mechanism to support what clang-tidy needs.
>
>   2) I find the grouping of checks under specific projects (Google and
> LLVM) to be odd. I think what we want is just a bunch of checks with
> descriptive names (explicit constructors, sorted includes, end-of-namespace
> comments, etc.), and some very lightweight mechanism that composes a set of
> checks (based on their names) into a "coding convention". That will be the
> point of configurability later, to compose sets of checks into your own
> coding style. Yes, there may be some weird checks that make sense for a
> particular project, and we can prefix those if we have to, but overall I
> expect most checks to be universal.
>

One thing I expect is that with the power of clang we'll see more projects
build checks that are domain specific to their internal infrastructure
classes.

Another point is configurability. While many standards have very similar
checks - for example that end of namespace comments follow a certain
pattern - they all look differently. We could of course just try to find
more descriptive names for what the option is, but that seems harder.

Or are you opposed to the general idea that we have a set of all possible
checks and basically a way to filter that down by regexp, and would prefer
a more declarative syntax for how to say which checks you want to run (and
perhaps with what configured values)?

Cheers,
/Manuel


>
>
> ================
> Comment at: clang-tidy/google/GoogleModule.cpp:37
> @@ +36,3 @@
> +    if (!Ctor->isExplicit() && !Ctor->isImplicit() &&
> +        Ctor->getNumParams() == 1) {
> +      SourceLocation Loc = Ctor->getLocation();
> ----------------
> This won't handle constructors that have > 1 arguments where the second
> and later arguments have defaults. Use Ctor->getNumParams() >= 1 &&
> Ctor->getMinRequiredArguments()  <= 1
>
> ================
> Comment at: clang-tidy/google/GoogleModule.cpp:42
> @@ +41,3 @@
> +          tooling::Replacement(*Result.SourceManager, Loc, 0, "explicit
> "));
> +      Context->reportError(ClangTidyError(
> +          *Result.SourceManager, Loc,
> ----------------
> Actually fixing this issue can break code. Is that something for the
> tooling infrastructure to deal with? Should it be indicated somehow in the
> diagnostic?
>
> ================
> Comment at: clang-tidy/llvm/LLVMModule.cpp:46
> @@ +45,3 @@
> +      // FIXME: Check comment content.
> +      IsProperlyTerminated = true;
> +    }
> ----------------
> Early return here.
>
> ================
> Comment at: clang-tidy/llvm/LLVMModule.cpp:51
> @@ +50,3 @@
> +      if (!ND->isAnonymousNamespace())
> +        Fix = Fix.append(" ").append(ND->getNameAsString());
> +      tooling::Replacements Replacements;
> ----------------
> else "Fix = " // anonymous namespace"; ?
>
>
> http://llvm-reviews.chandlerc.com/D884
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130605/65e01bda/attachment.html>