This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Simplify ToolChain::GetCXXStdlibType (NFC)
ClosedPublic

Authored by Hahnfeld on Oct 17 2016, 1:28 AM.

Details

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.

Diff Detail

Event Timeline

Hahnfeld updated this revision to Diff 74818.Oct 17 2016, 1:28 AM
Hahnfeld retitled this revision from to [Driver] Simplify ToolChain::GetCXXStdlibType (NFC).
Hahnfeld updated this object.
Hahnfeld added reviewers: mgorny, ddunbar, phosek.
Hahnfeld added subscribers: cfe-commits, zlei.
ABataev added inline comments.
lib/Driver/ToolChain.cpp
553–559

I believe you can use StringSwitch here

mgorny edited edge metadata.Oct 17 2016, 8:44 AM

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

lib/Driver/ToolChain.cpp
553

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 updated this revision to Diff 74948.Oct 17 2016, 11:32 PM
Hahnfeld edited edge metadata.
Hahnfeld marked an inline comment as done.

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

lib/Driver/ToolChain.cpp
553–559

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...

ABataev added inline comments.Oct 26 2016, 6:12 AM
lib/Driver/ToolChain.cpp
553–559

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 added inline comments.Oct 26 2016, 6:33 AM
lib/Driver/ToolChain.cpp
553–559

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...

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

This revision was automatically updated to reflect the committed changes.