[PATCH] Allow per-file clang-tidy options.

Alexander Kornienko alexfh at google.com
Mon Jun 2 15:46:36 PDT 2014


================
Comment at: clang-tidy/ClangTidy.cpp:208
@@ +207,3 @@
+
+  SmallVector<ClangTidyCheck*, 8> Checks;
+  ChecksFilter &Filter = Context.getChecksFilter();
----------------
Manuel Klimek wrote:
> Any reasons for not using a vector of unique_ptr's an moving that into the ASTConsumer?
Just didn't want to touch the createChecks method. But this isn't a serious reason. Fixed.

================
Comment at: clang-tidy/ClangTidy.cpp:216
@@ -204,1 +215,3 @@
+    Check->setContext(&Context);
+    Check->registerMatchers(&*Finder);
     Check->registerPPCallbacks(Compiler);
----------------
Manuel Klimek wrote:
> Finder.get()?
It's a matter of taste. I slightly prefer &*, and &* is used two more times in this file (lines 98, 99) and in other files in clang-tidy. If you have serious reasons to prefer .get() over &*, I can change all of the usages in a separate patch, but now I would like the code to be consistent in this regard.

================
Comment at: clang-tidy/ClangTidy.cpp:269
@@ +268,3 @@
+    AnalyzerChecksEnabled =
+        AnalyzerChecksEnabled | !CheckName.startswith("debug") &&
+        Filter.isCheckEnabled(Checker);
----------------
Manuel Klimek wrote:
> Wait, this seems to have been incorrect before (| instead of logical ||), and I think we're missing parens.
Err, it's actually incorrect now (| instead of ||). And we're missing parens (to mute the compiler warning). Before this patch it was kind of correct (except that it wasn't short circuited).

Fixed.

================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.h:170
@@ +169,3 @@
+
+  StringRef CurrentFile;
+  std::unique_ptr<ChecksFilter> CheckFilter;
----------------
Manuel Klimek wrote:
> Any reason not to store a std::string here? Seems fragile to store a StringRef.
You're right. Changed.

================
Comment at: clang-tidy/ClangTidyOptions.h:30-31
@@ -25,1 +29,4 @@
+
+  /// \brief A list of line ranges in this file, from where the warnings may be
+  /// displayed.
   std::vector<LineRange> LineRanges;
----------------
Manuel Klimek wrote:
> ... for which we show warnings.
Done.

================
Comment at: clang-tidy/ClangTidyOptions.h:35
@@ -27,2 +34,3 @@
 
-/// \brief Contains options for clang-tidy.
+/// \brief Global options. These options are nor stored neither read from
+/// configuration files.
----------------
Manuel Klimek wrote:
> neither... nor...
Done.

================
Comment at: clang-tidy/ClangTidyOptions.h:38-39
@@ +37,4 @@
+struct ClangTidyGlobalOptions {
+  /// \brief Output warnings from certain line ranges of certain files only. If
+  /// this list is emtpy, it won't be applied.
+  std::vector<FileFilter> LineFilter;
----------------
Manuel Klimek wrote:
> I think the second sentence doesn't help. I'd delete it.
I'd leave it, as it's describes a special case. Without special-casing the empty list, the list would be applied always, and the empty list would mean that no warnings will pass the filter.

================
Comment at: clang-tidy/ClangTidyOptions.h:76-78
@@ +75,5 @@
+class DefaultOptionsProvider : public ClangTidyOptionsProvider {
+  ClangTidyGlobalOptions GlobalOptions;
+  ClangTidyOptions DefaultOptions;
+
+public:
----------------
Manuel Klimek wrote:
> I'd vote for putting members at the end of the class.
Done.

http://reviews.llvm.org/D3979






More information about the cfe-commits mailing list