[PATCH] Validate clang-tidy check names.

Daniel Jasper djasper at google.com
Wed Aug 20 07:15:22 PDT 2014


================
Comment at: clang-tidy/ClangTidy.cpp:294
@@ -292,1 +293,3 @@
 
+      if (!ClangTidyCheck::isValidCheckName(Checker)) {
+        assert(false &&
----------------
Should we really validate the analyzer check names here? And if so, should we assert?

I'd be more comfortable moving at least the assert to somewhere in the static analyzer itself, so that people working on it know what valid names are and don't have to discover that by breaking an assert build of clang-tidy.

Or, alternatively, people can't add more analyzer checks by linking additional libraries to clang-tidy, right? Should we just have a test that calls "clang-tidy -list-checks" and verifies all the names?

================
Comment at: clang-tidy/ClangTidy.cpp:323
@@ +322,3 @@
+bool ClangTidyCheck::isValidCheckName(StringRef Name) {
+  return llvm::Regex("^[a-zA-Z0-9_.\\-]+$").match(Name);
+}
----------------
It almost feels like it is not worth implementing and testing this function. I'd be fine with simply defining the string as a constant and then calling llvm::Regex().match() at the two call sites. Specifically, if it is located here, people might think that they have to do something with this function when writing a check.

================
Comment at: clang-tidy/ClangTidy.h:89
@@ -88,1 +88,3 @@
 
+  /// \brief Returns \c true, if the specified string can be used for a check
+  /// name.
----------------
Maybe: .., if \p Name can be used as a check name.

http://reviews.llvm.org/D4982






More information about the cfe-commits mailing list