Could you include more context with the patch? See: http://llvm.org/docs/Phabricator.html
Looks like we don't have a public API for options with string values. Thanks! Anna. ================ Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:275 @@ +274,3 @@ + /// ones. + ///// @retval CheckerOptionValue An option for a checker if it was specified + /// @retval GroupOptionValue An option for group if it was specified and no ---------------- Extra "//". Nit: Please, make sure all comments have proper punctuation. (A period is missing here and in a few other places.) ================ Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:288 @@ -257,3 +287,3 @@ /// /// Accepts the strings "true" and "false". /// If an option value is not provided, returns the given \p DefaultVal. ---------------- This is a bit confusing. ================ Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:311 @@ +310,3 @@ + /// @param [in] C Optional parameter that may be used to retrieve + /// checker-related option for a given checker + /// @param [in] SearchInParents If set to true and the searched option was not ---------------- This might be more clear: "The optional checker parameter that can be used to restrict the search to the options of this particular checker." ================ Comment at: lib/Frontend/CompilerInvocation.cpp:140 @@ -134,1 +139,3 @@ +} + static bool ParseAnalyzerArgs(AnalyzerOptions &Opts, ArgList &Args, ---------------- Shouldn't we use an existing clang check to check if substrings are identifiers? ================ Comment at: lib/StaticAnalyzer/Core/AnalyzerOptions.cpp:110 @@ +109,3 @@ + // Search for a category option if option for checker is not specified and + // inheritance is enabled. + ConfigTable::const_iterator E = Config.end(); ---------------- The comment needs to move and be rephrased (there is no inheritance and checkers are organized in packages, not categories). ================ Comment at: unittests/Analysis/AnalyzerOptionsTest.cpp:47 @@ +46,2 @@ +} // end namespace ast_matchers +} // end namespace clang ---------------- I think we need more tests. Ex: overridden options, options set only in a package. ================ Comment at: unittests/CMakeLists.txt:16 @@ -15,2 +15,3 @@ if(CLANG_ENABLE_STATIC_ANALYZER) + add_subdirectory(Analysis) add_subdirectory(Frontend) ---------------- "Analysis" is too vague if we only plan to use this for the clang static analyzer. http://reviews.llvm.org/D7905 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
