[analyzer][NFC] Fix inconsistencies in AnalyzerOptions

[Szelethus]

Summary

I was a little unsure about the title, but it is what it is.

I'm in the process of refactoring AnalyzerOptions. The main motivation behind here is to emit warnings if an invalid -analyzer-config option is given from the command line, and be able to list them all.

This first NFC patch contains small modifications to make AnalyzerOptions.cpp a little more consistent.

[george.karpenkov]

The main motivation behind here is to emit warnings if an invalid

I'm totally with you here, but IIRC (@NoQ might want to correct me here),
the design decision was made specifically to ignore incorrect options, so that e.g. old versions of Xcode used with old projects would still work.

[george.karpenkov]

I'm not sure why you could get away with removing those llvm_unreachable cases?

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

373 ↗

(On Diff #169703)

Why?

418 ↗

(On Diff #169703)

Why?

lib/StaticAnalyzer/Core/CoreEngine.cpp

72 ↗

(On Diff #169703)

Why?

[Szelethus]

In D53274#1265625, @george.karpenkov wrote:

I'm not sure why you could get away with removing those llvm_unreachable cases?

Because I got a warning for using default when every enum value was handled in the switch. Since whether the flag is set or not can be retrieved via llvm::Optional::hasValue(), I removed the .*NotSet flags. The future maintainer who adds a new value will get a warning for not handling it anyways.

In D53274#1265618, @george.karpenkov wrote:

The main motivation behind here is to emit warnings if an invalid

I'm totally with you here, but IIRC (@NoQ might want to correct me here),
the design decision was made specifically to ignore incorrect options, so that e.g. old versions of Xcode used with old projects would still work.

I intend to emit a warning, but go on with the analysis. That's fair I believe?

[george.karpenkov]

OK that makes sense to me, just let's be careful not to crash in places where we weren't crashing before.

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

354 ↗

(On Diff #169703)

I'm slightly confused, where is this function called?
Same for the function above?

[Szelethus]

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

354 ↗

(On Diff #169703)

Its used in D53277, but this addition belongs here more.

Granted, I should've refactored this file to demonstrate why I made this change, but in D53277 I literally delete everything, so I didn't bother.

[NoQ]

Thanks! Let's commit.

cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

 /// considered for inlining by the analyzer.
 ///
 /// These options are cumulative; enabling one kind of member function will
 /// enable all kinds with lower enum values.
 enum CXXInlineableMemberKind {
   // Uninitialized = 0,
 
   /// A dummy mode in which no C++ inlining is enabled.
-  CIMK_None = 1,
+  CIMK_None,
 
   /// Refers to regular member function and operator calls.
   CIMK_MemberFunctions,
 
   /// Refers to constructors (implicit or explicit).
   ///
   /// Note that a constructor will not be inlined if the corresponding
   /// destructor is non-trivial.
   CIMK_Constructors,
 
   /// Refers to destructors (implicit or explicit).
   CIMK_Destructors
 };
 
 /// Describes the different modes of inter-procedural analysis.
 enum IPAKind {
-  IPAK_NotSet = 0,
-
   /// Perform only intra-procedural analysis.
   IPAK_None = 1,
 
   /// Inline C functions and blocks when their definitions are available.
   IPAK_BasicInlining = 2,
 
   /// Inline callees(C, C++, ObjC) when their definitions are available.
   IPAK_Inlining = 3,
public:
 
   enum class ExplorationStrategyKind {
     DFS,
     BFS,
     UnexploredFirst,
     UnexploredFirstQueue,
     UnexploredFirstLocationQueue,
     BFSBlockDFSContents,
-    NotSet
   };
 
 private:
-  ExplorationStrategyKind ExplorationStrategy = ExplorationStrategyKind::NotSet;
-
   /// Describes the kinds for high-level analyzer mode.
   enum UserModeKind {
-    UMK_NotSet = 0,
-
     /// Perform shallow but fast analyzes.
     UMK_Shallow = 1,
 
     /// Perform deep analyzes.
     UMK_Deep = 2
   };
 
+  llvm::Optional<ExplorationStrategyKind> ExplorationStrategy;
+
   /// Controls the high-level analyzer mode, which influences the default
   /// settings for some of the lower-level config options (such as IPAMode).
   /// \sa getUserMode
-  UserModeKind UserMode = UMK_NotSet;
+  llvm::Optional<UserModeKind> UserMode;
 
   /// Controls the mode of inter-procedural analysis.
-  IPAKind IPAMode = IPAK_NotSet;
+  llvm::Optional<IPAKind> IPAMode;
 
   /// Controls which C++ member functions will be considered for inlining.
-  CXXInlineableMemberKind CXXMemberInliningMode;
+  llvm::Optional<CXXInlineableMemberKind> CXXMemberInliningMode;
 
   /// \sa includeImplicitDtorsInCFG
   Optional<bool> IncludeImplicitDtorsInCFG;
 
   /// \sa includeTemporaryDtorsInCFG
   Optional<bool> IncludeTemporaryDtorsInCFG;
 
   /// \sa IncludeLifetimeInCFG
public:
   /// @param [in] SearchInParents If set to true and the searched option was not
   /// specified for the given checker the options for the parent packages will
   /// be searched as well. The inner packages take precedence over the outer
   /// ones.
   int getOptionAsInteger(StringRef Name, int DefaultVal,
                          const ento::CheckerBase *C = nullptr,
                          bool SearchInParents = false);
 
+
+  unsigned getOptionAsUInt(Optional<unsigned> &V, StringRef Name,
+                           unsigned DefaultVal,
+                           const ento::CheckerBase *C = nullptr,
+                           bool SearchInParents = false);
+
   /// Query an option's string value.
   ///
   /// If an option value is not provided, returns the given \p DefaultVal.
   /// @param [in] Name Name for option to retrieve.
   /// @param [in] DefaultVal Default value returned if no such option was
   /// specified.
   /// @param [in] C The optional checker parameter that can be used to restrict
   /// the search to the options of this particular checker (and its parents
   /// depending on search mode).
   /// @param [in] SearchInParents If set to true and the searched option was not
   /// specified for the given checker the options for the parent packages will
   /// be searched as well. The inner packages take precedence over the outer
   /// ones.
   StringRef getOptionAsString(StringRef Name, StringRef DefaultVal,
                               const ento::CheckerBase *C = nullptr,
                               bool SearchInParents = false);
 
+  StringRef getOptionAsString(Optional<StringRef> &V, StringRef Name,
+                              StringRef DefaultVal,
+                              const ento::CheckerBase *C = nullptr,
+                              bool SearchInParents = false);
+
   /// Retrieves and sets the UserMode. This is a high-level option,
   /// which is used to set other low-level options. It is not accessible
   /// outside of AnalyzerOptions.
   UserModeKind getUserMode();
 
   ExplorationStrategyKind getExplorationStrategy();
 
   /// Returns the inter-procedural analysis mode.

cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

for (StringRef CheckName : StaticAnalyzerChecks) {
     if (!CheckName.startswith("debug.") &&
         (IncludeExperimental || !CheckName.startswith("alpha.")))
       Result.push_back(CheckName);
   }
   return Result;
 }
 
 AnalyzerOptions::UserModeKind AnalyzerOptions::getUserMode() {
-  if (UserMode == UMK_NotSet) {
-    StringRef ModeStr =
-        Config.insert(std::make_pair("mode", "deep")).first->second;
-    UserMode = llvm::StringSwitch<UserModeKind>(ModeStr)
+  if (!UserMode.hasValue()) {
+    StringRef ModeStr = getOptionAsString("mode", "deep");
+    UserMode = llvm::StringSwitch<llvm::Optional<UserModeKind>>(ModeStr)
       .Case("shallow", UMK_Shallow)
       .Case("deep", UMK_Deep)
-      .Default(UMK_NotSet);
-    assert(UserMode != UMK_NotSet && "User mode is invalid.");
+      .Default(None);
+    assert(UserMode.getValue() && "User mode is invalid.");
   }
-  return UserMode;
+  return UserMode.getValue();
 }
 
 AnalyzerOptions::ExplorationStrategyKind
 AnalyzerOptions::getExplorationStrategy() {
-  if (ExplorationStrategy == ExplorationStrategyKind::NotSet) {
-    StringRef StratStr =
-        Config
-            .insert(std::make_pair("exploration_strategy", "unexplored_first_queue"))
-            .first->second;
+  if (!ExplorationStrategy.hasValue()) {
+    StringRef StratStr = getOptionAsString("exploration_strategy",
+                                           "unexplored_first_queue");
     ExplorationStrategy =
-        llvm::StringSwitch<ExplorationStrategyKind>(StratStr)
+        llvm::StringSwitch<llvm::Optional<ExplorationStrategyKind>>(StratStr)
             .Case("dfs", ExplorationStrategyKind::DFS)
             .Case("bfs", ExplorationStrategyKind::BFS)
             .Case("unexplored_first",
                   ExplorationStrategyKind::UnexploredFirst)
             .Case("unexplored_first_queue",
                   ExplorationStrategyKind::UnexploredFirstQueue)
             .Case("unexplored_first_location_queue",
                   ExplorationStrategyKind::UnexploredFirstLocationQueue)
             .Case("bfs_block_dfs_contents",
                   ExplorationStrategyKind::BFSBlockDFSContents)
-            .Default(ExplorationStrategyKind::NotSet);
-    assert(ExplorationStrategy != ExplorationStrategyKind::NotSet &&
+            .Default(None);
+    assert(ExplorationStrategy.hasValue() &&
            "User mode is invalid.");
   }
-  return ExplorationStrategy;
+  return ExplorationStrategy.getValue();
 }
 
 IPAKind AnalyzerOptions::getIPAMode() {
-  if (IPAMode == IPAK_NotSet) {
+  if (!IPAMode.hasValue()) {
     // Use the User Mode to set the default IPA value.
     // Note, we have to add the string to the Config map for the ConfigDumper
     // checker to function properly.
     const char *DefaultIPA = nullptr;
     UserModeKind HighLevelMode = getUserMode();
     if (HighLevelMode == UMK_Shallow)
       DefaultIPA = "inlining";
     else if (HighLevelMode == UMK_Deep)
       DefaultIPA = "dynamic-bifurcate";
     assert(DefaultIPA);
 
     // Lookup the ipa configuration option, use the default from User Mode.
-    StringRef ModeStr =
-        Config.insert(std::make_pair("ipa", DefaultIPA)).first->second;
-    IPAKind IPAConfig = llvm::StringSwitch<IPAKind>(ModeStr)
+    StringRef ModeStr = getOptionAsString("ipa", DefaultIPA);
+    IPAMode = llvm::StringSwitch<llvm::Optional<IPAKind>>(ModeStr)
             .Case("none", IPAK_None)
             .Case("basic-inlining", IPAK_BasicInlining)
             .Case("inlining", IPAK_Inlining)
             .Case("dynamic", IPAK_DynamicDispatch)
             .Case("dynamic-bifurcate", IPAK_DynamicDispatchBifurcate)
-            .Default(IPAK_NotSet);
-    assert(IPAConfig != IPAK_NotSet && "IPA Mode is invalid.");
-
-    // Set the member variable.
-    IPAMode = IPAConfig;
+            .Default(None);
+    assert(IPAMode.hasValue() && "IPA Mode is invalid.");
   }
 
-  return IPAMode;
+  return IPAMode.getValue();
 }
 
 bool
 AnalyzerOptions::mayInlineCXXMemberFunction(CXXInlineableMemberKind K) {
   if (getIPAMode() < IPAK_Inlining)
     return false;
 
   if (!CXXMemberInliningMode) {
-    static const char *ModeKey = "c++-inlining";
-
-    StringRef ModeStr =
-        Config.insert(std::make_pair(ModeKey, "destructors")).first->second;
-
-    CXXInlineableMemberKind &MutableMode =
-      const_cast<CXXInlineableMemberKind &>(CXXMemberInliningMode);
+    StringRef ModeStr = getOptionAsString("c++-inlining", "destructors");
 
-    MutableMode = llvm::StringSwitch<CXXInlineableMemberKind>(ModeStr)
+    CXXMemberInliningMode =
+      llvm::StringSwitch<llvm::Optional<CXXInlineableMemberKind>>(ModeStr)
       .Case("constructors", CIMK_Constructors)
       .Case("destructors", CIMK_Destructors)
-      .Case("none", CIMK_None)
       .Case("methods", CIMK_MemberFunctions)
-      .Default(CXXInlineableMemberKind());
+      .Case("none", CIMK_None)
+      .Default(None);
 
-    if (!MutableMode) {
-      // FIXME: We should emit a warning here about an unknown inlining kind,
-      // but the AnalyzerOptions doesn't have access to a diagnostic engine.
-      MutableMode = CIMK_None;
-    }
+    assert(CXXMemberInliningMode.hasValue() &&
+           "Invalid c++ member function inlining mode.");
   }
 
-  return CXXMemberInliningMode >= K;
+  return *CXXMemberInliningMode >= K;
 }
 
 static StringRef toString(bool b) { return b ? "true" : "false"; }
 
 StringRef AnalyzerOptions::getCheckerOption(StringRef CheckerName,
                                             StringRef OptionName,
                                             StringRef Default,
                                             bool SearchInParents) {
bool AnalyzerOptions::getBooleanOption(StringRef Name, bool DefaultVal,
                                        bool SearchInParents) {
   // FIXME: We should emit a warning here if the value is something other than
   // "true", "false", or the empty string (meaning the default value),
   // but the AnalyzerOptions doesn't have access to a diagnostic engine.
   StringRef Default = toString(DefaultVal);
   StringRef V =
       C ? getCheckerOption(C->getTagDescription(), Name, Default,
                            SearchInParents)
-        : StringRef(Config.insert(std::make_pair(Name, Default)).first->second);
+        : getOptionAsString(Name, Default);
   return llvm::StringSwitch<bool>(V)
       .Case("true", true)
       .Case("false", false)
       .Default(DefaultVal);
 }
 
 bool AnalyzerOptions::getBooleanOption(Optional<bool> &V, StringRef Name,
                                        bool DefaultVal, const CheckerBase *C,
int AnalyzerOptions::getOptionAsInteger(StringRef Name, int DefaultVal,
                                         const CheckerBase *C,
                                         bool SearchInParents) {
   SmallString<10> StrBuf;
   llvm::raw_svector_ostream OS(StrBuf);
   OS << DefaultVal;
 
   StringRef V = C ? getCheckerOption(C->getTagDescription(), Name, OS.str(),
                                      SearchInParents)
-                  : StringRef(Config.insert(std::make_pair(Name, OS.str()))
-                                  .first->second);
+                  : getOptionAsString(Name, OS.str());
 
   int Res = DefaultVal;
   bool b = V.getAsInteger(10, Res);
   assert(!b && "analyzer-config option should be numeric");
   (void)b;
   return Res;
 }
 
+unsigned AnalyzerOptions::getOptionAsUInt(Optional<unsigned> &V, StringRef Name,
+                                          unsigned DefaultVal,
+                                          const CheckerBase *C,
+                                          bool SearchInParents) {
+  if (!V.hasValue())
+    V = getOptionAsInteger(Name, DefaultVal, C, SearchInParents);
+  return V.getValue();
+}
+
 StringRef AnalyzerOptions::getOptionAsString(StringRef Name,
                                              StringRef DefaultVal,
                                              const CheckerBase *C,
                                              bool SearchInParents) {
   return C ? getCheckerOption(C->getTagDescription(), Name, DefaultVal,
                               SearchInParents)
            : StringRef(
                  Config.insert(std::make_pair(Name, DefaultVal)).first->second);
 }
 
+StringRef AnalyzerOptions::getOptionAsString(Optional<StringRef> &V,
+                                             StringRef Name,
+                                             StringRef DefaultVal,
+                                             const ento::CheckerBase *C,
+                                             bool SearchInParents) {
+  if (!V.hasValue())
+    V = getOptionAsString(Name, DefaultVal, C, SearchInParents);
+  return V.getValue();
+}
+
 unsigned AnalyzerOptions::getAlwaysInlineSize() {
   if (!AlwaysInlineSize.hasValue())
     AlwaysInlineSize = getOptionAsInteger("ipa-always-inline-size", 3);
   return AlwaysInlineSize.getValue();
 }
 
 unsigned AnalyzerOptions::getMaxInlinableSize() {
   if (!MaxInlinableSize.hasValue()) {
     int DefaultValue = 0;
     UserModeKind HighLevelMode = getUserMode();
     switch (HighLevelMode) {
-      default:
-        llvm_unreachable("Invalid mode.");
       case UMK_Shallow:
         DefaultValue = 4;
         break;
       case UMK_Deep:
         DefaultValue = 100;
         break;
     }
 
unsigned AnalyzerOptions::getMinCFGSizeTreatFunctionsAsLarge() {
   return MinCFGSizeTreatFunctionsAsLarge.getValue();
 }
 
 unsigned AnalyzerOptions::getMaxNodesPerTopLevelFunction() {
   if (!MaxNodesPerTopLevelFunction.hasValue()) {
     int DefaultValue = 0;
     UserModeKind HighLevelMode = getUserMode();
     switch (HighLevelMode) {
-      default:
-        llvm_unreachable("Invalid mode.");
       case UMK_Shallow:
         DefaultValue = 75000;
         break;
       case UMK_Deep:
         DefaultValue = 225000;
         break;
     }
     MaxNodesPerTopLevelFunction = getOptionAsInteger("max-nodes", DefaultValue);

cfe/trunk/lib/StaticAnalyzer/Core/CoreEngine.cpp

switch (Opts.getExplorationStrategy()) {
     case AnalyzerOptions::ExplorationStrategyKind::BFSBlockDFSContents:
       return WorkList::makeBFSBlockDFSContents();
     case AnalyzerOptions::ExplorationStrategyKind::UnexploredFirst:
       return WorkList::makeUnexploredFirst();
     case AnalyzerOptions::ExplorationStrategyKind::UnexploredFirstQueue:
       return WorkList::makeUnexploredFirstPriorityQueue();
     case AnalyzerOptions::ExplorationStrategyKind::UnexploredFirstLocationQueue:
       return WorkList::makeUnexploredFirstPriorityLocationQueue();
-    default:
-      llvm_unreachable("Unexpected case");
   }
 }
 
 CoreEngine::CoreEngine(SubEngine &subengine, FunctionSummariesTy *FS,
                        AnalyzerOptions &Opts)
     : SubEng(subengine), WList(generateWorkList(Opts, subengine)),
       BCounterFactory(G.getAllocator()), FunctionSummaries(FS) {}