Cool, looks good.
================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:50
@@ +49,3 @@
+ "set of enabled checks.\n"
+ "This option's value is appended to the value read\n"
+ "from a .clang-tidy file, if any."),
----------------
alexfh wrote:
> djasper wrote:
> > alexfh wrote:
> > > djasper wrote:
> > > > I am not entirely certain that appending is the right thing to do here.
> > > > But I understand that otherwise, it would be next to impossible to say
> > > > "everything in the config file +/- this one check". Then again, with
> > > > -dump-config, it might be easy enough to copy and past the existing
> > > > active checks for a given file and modify them.
> > > This is done to keep the current behavior. If we want to change it, I'd
> > > rather do this as a separate step. And we also need strong reasons to do
> > > so, as it would degrade user experience, imo.
> > This can't be the current behavior as currently, there are no .clang-tidy
> > files. And that's also why I want us to think about it now, before we have
> > to break existing behavior. In your opinion, what are the pros/cons
> > for/against appending?
> The current behavior is that adding -checks=... on the command line modifies
> the set of checks by appending the new value to the value that would be used
> without the command line option. The new code behaves exactly in the same way
> when there's no .clang-tidy file, and naturally extends the same behavior to
> the case when there is a .clang-tidy file (the "defaults" are taken from the
> .clang-tidy file instead).
>
> If instead of appending we overwrite the value, we'll change the behavior of
> clang-tidy in both use cases.
>
> Apart from trying to avoid changes in behavior, I see the following benefit
> of the current approach: it allows to slightly change the default set of
> checks as well as completely override it equally easily (use
> -checks=some-check to add a check to the default set, -checks=-some-check to
> remove it, -checks=-*,some-check to only run some-check).
Ok. I am fine with it, I just want to be sure about the implications.
================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:91
@@ +90,3 @@
+
+static cl::opt<bool> AnalyzeTemporaryDtors(
+ "analyze-temporary-dtors",
----------------
alexfh wrote:
> djasper wrote:
> > alexfh wrote:
> > > djasper wrote:
> > > > Hm. I am wondering whether it really makes sense to keep these
> > > > individual config flags. The approach in clang-format, where we can
> > > > just pass in a JSON config through a single flag seems like a good
> > > > alternative.
> > > I'd keep at least the most frequently used options, e.g. -checks:
> > >
> > > clang-tidy -config='{Checks: "..."}'
> > >
> > > instead of
> > >
> > > clang-tidy -checks="..."
> > >
> > > is not extremely ugly, but a bit less convenient. Anyways, I'd like to
> > > make any unnecessary behavior changes in a separate patch.
> > Ok, but what are the more frequently used flags? Also, dumping the current
> > config into a .clang-tidy file and changing that seems like the more
> > convenient way to change these things anyway.
> >
> > FWIW, I somewhat agree on keeping "-checks" (although I have no data on how
> > frequently it's used). However, I don't thing -analyze-temporary-dtors is
> > or should be a "frequent flag", which is why I left the comment here.
> I totally agree with the idea of removing -analyze-temporary-dtors if we add
> a command-line option to specify JSON config. The -header-filter= option also
> doesn't look like a frequently used. However, -checks= is used at least by
> the check developers and in all our documents, so we'd better leave it.
Makes sense.
http://reviews.llvm.org/D5186
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
