[PATCH] [clang-tidy] Add a checker for long functions.

Alexander Kornienko alexfh at google.com
Fri Sep 12 09:46:14 PDT 2014


================
Comment at: clang-tidy/misc/FunctionSize.cpp:21
@@ +20,3 @@
+      LineThreshold(Options.get("LineThreshold", -1U)),
+      StatementThreshold(Options.get("StatementThreshold", 800)),
+      BranchThreshold(Options.get("BranchThreshold", -1U)) {}
----------------
Please make it 800U to instantiate get<unsigned>().

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:108
@@ +107,3 @@
+    ConfigFile("config",
+               cl::desc("File to override the configuration. Specifing a\nfile "
+                        "will disable the default configuration file\nsearch."),
----------------
1. Please move configuration-related changes to a separate patch.
2. Please break the strings after \n's (and maybe file a bug against clang-format to respect \n's located close to the column limit).
3. s/Specifing/Specifying/
4. It seems reasonable to allow inline configurations like in clang-format, if the -config= value starts with a '{':


  clang-tidy -config='{CheckOptions: [{key: misc-function-size.LineThreshold, value: 0}, {key: misc-function-size.StatementThreshold, value: 0}]}' some-file.cpp

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:178
@@ +177,3 @@
+    if (std::error_code EC =
+            parseConfiguration((*Text)->getBuffer(), EffectiveOptions)) {
+      llvm::errs() << "Error reading config file: " << EC.message() << '\n';
----------------
EffectiveOptions is only used for -list-checks and -dump-config. ClangTidy itself uses OptionsProvider, so overriding configuration file needs to be done in FileOptionsProvider. Alternatively, you can do:

  OptionsProvider.reset(new DefaultOptionsProvider(GlobalOptions, EffectiveOptions));

after this. I'd probably prefer the first way.

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:182
@@ +181,3 @@
+    }
+    EffectiveOptions.mergeWith(OverrideOptions);
+  } else {
----------------
This line doesn't do what you expect (mergeWith should probably have LLVM_ATTRIBUTE_UNUSED_RESULT).

http://reviews.llvm.org/D4986






More information about the cfe-commits mailing list