[Driver] Simplify ToolChain::GetCXXStdlibType (NFC)

[Hahnfeld]

Summary

I made the wrong assumption that execution would continue after an error Diag which led to unneeded complex code.
This patch aligns with the better implementation of ToolChain::GetRuntimeLibType.

[ABataev]

lib/Driver/ToolChain.cpp

553–559 ↗

(On Diff #74818)

I believe you can use StringSwitch here

[mgorny]

Thanks for working on this. Your code is much cleaner ;-).

lib/Driver/ToolChain.cpp

553 ↗

(On Diff #74818)

I think you are changing the meaning of this comment. The original sounded like a request not to use this type anywhere else. Your version only explains where it's used (right now).

[Hahnfeld]

Update comments to express that platform should only be used in tests

lib/Driver/ToolChain.cpp

553–559 ↗

(On Diff #74818)

I'm uncertain how to apply it while also preserving the Diag for an invalid argument: I would probably need an additional CST_Unknown which I don't like more...

[Hahnfeld]

gentle ping

[ABataev]

lib/Driver/ToolChain.cpp

553–559 ↗

(On Diff #74818)

I think you can try something like this:

llvm::Optional<ToolChain::CXXStdlibType> Res = StringSwitch<ToolChain::CXXStdlibType, llvm::NoneType>(LibName).Case("libc++", ToolChain::CST_Libcxx).Case("libstdc++", ToolChain::CST_Libstdcxx).Case("platform", GetDefaultCXXStdlibType()).Default(llvm::None);
if (Res)
  return Res.getValue();

[Hahnfeld]

lib/Driver/ToolChain.cpp

553–559 ↗

(On Diff #74818)

Does that really improve readability? StringSwitch is currently never used in ToolChain.cpp and the cascade with ifs is one line shorter in this case...

[Hahnfeld]

Can I commit this as-is for now and we can think about StringSwitch in the driver later on?

[Hahnfeld]

ping

[Hahnfeld]

ping

[Hahnfeld]

Ping!

[zlei]

LGTM

cfe/trunk/lib/Driver/ToolChain.cpp

void ToolChain::addProfileRTLibs(const llvm::opt::ArgList &Args,
   CmdArgs.push_back(getCompilerRTArgString(Args, "profile"));
 }
 
 ToolChain::RuntimeLibType ToolChain::GetRuntimeLibType(
     const ArgList &Args) const {
   const Arg* A = Args.getLastArg(options::OPT_rtlib_EQ);
   StringRef LibName = A ? A->getValue() : CLANG_DEFAULT_RTLIB;
 
-  // "platform" is only used in tests to override CLANG_DEFAULT_RTLIB
+  // Only use "platform" in tests to override CLANG_DEFAULT_RTLIB!
   if (LibName == "compiler-rt")
     return ToolChain::RLT_CompilerRT;
   else if (LibName == "libgcc")
     return ToolChain::RLT_Libgcc;
   else if (LibName == "platform")
     return GetDefaultRuntimeLibType();
 
   if (A)
     getDriver().Diag(diag::err_drv_invalid_rtlib_name) << A->getAsString(Args);
 
   return GetDefaultRuntimeLibType();
 }
 
-static bool ParseCXXStdlibType(const StringRef& Name,
-                               ToolChain::CXXStdlibType& Type) {
-  if (Name == "libc++")
-    Type = ToolChain::CST_Libcxx;
-  else if (Name == "libstdc++")
-    Type = ToolChain::CST_Libstdcxx;
-  else
-    return false;
-
-  return true;
-}
-
 ToolChain::CXXStdlibType ToolChain::GetCXXStdlibType(const ArgList &Args) const{
-  ToolChain::CXXStdlibType Type;
-  bool HasValidType = false;
-  bool ForcePlatformDefault = false;
-
   const Arg *A = Args.getLastArg(options::OPT_stdlib_EQ);
-  if (A) {
-    StringRef Value = A->getValue();
-    HasValidType = ParseCXXStdlibType(Value, Type);
-
-    // Only use in tests to override CLANG_DEFAULT_CXX_STDLIB!
-    if (Value == "platform")
-      ForcePlatformDefault = true;
-    else if (!HasValidType)
-      getDriver().Diag(diag::err_drv_invalid_stdlib_name)
-        << A->getAsString(Args);
-  }
-
-  // If no argument was provided or its value was invalid, look for the
-  // default unless forced or configured to take the platform default.
-  if (!HasValidType && (ForcePlatformDefault ||
-      !ParseCXXStdlibType(CLANG_DEFAULT_CXX_STDLIB, Type)))
-    Type = GetDefaultCXXStdlibType();
+  StringRef LibName = A ? A->getValue() : CLANG_DEFAULT_CXX_STDLIB;
+
+  // Only use "platform" in tests to override CLANG_DEFAULT_CXX_STDLIB!
+  if (LibName == "libc++")
+    return ToolChain::CST_Libcxx;
+  else if (LibName == "libstdc++")
+    return ToolChain::CST_Libstdcxx;
+  else if (LibName == "platform")
+    return GetDefaultCXXStdlibType();
+
+  if (A)
+    getDriver().Diag(diag::err_drv_invalid_stdlib_name) << A->getAsString(Args);
 
-  return Type;
+  return GetDefaultCXXStdlibType();
 }
 
 /// \brief Utility function to add a system include directory to CC1 arguments.
 /*static*/ void ToolChain::addSystemInclude(const ArgList &DriverArgs,
                                             ArgStringList &CC1Args,
                                             const Twine &Path) {
   CC1Args.push_back("-internal-isystem");
   CC1Args.push_back(DriverArgs.MakeArgString(Path));