[PATCH] D27477: Refactor how the MSVC toolchain searches for a compatibility version.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

This seems like a nice refactoring, and I'm pretty sure there's no real functional change in practice.



================
Comment at: include/clang/Driver/ToolChain.h:446
+  /// \brief On Windows, returns the MSVC compatibility version.
+  virtual VersionTuple getMSVCVersion(const Driver *D,
+                                      const llvm::opt::ArgList &Args) const {
----------------
This isn't a cheap operation, and we don't cache it. Let's call it something like `computeMSVCVersion`. I don't want us to read the version out of cl.exe more than once. Besides, it mirrors `ComputeLLVMTriple` etc.


================
Comment at: lib/Driver/MSVCToolChain.cpp:476-478
+namespace {
+
+VersionTuple getMSCompatibilityVersion(unsigned Version) {
----------------
I know this is more of an RFC patch, but I'll note we prefer `static` on free functions in case you hadn't encountered that http://llvm.org/docs/CodingStandards.html#id53

Also, this is poorly named. It's really building a version tuple from an unseparated, possibly full, version number. So, maybe `inferMSVCVersionSeparators` or `separateMSVCFullVersion` or something?


================
Comment at: lib/Driver/Tools.cpp:5739
 
   // -fms-compatibility-version=18.00 is default.
+  VersionTuple MSVT = getToolChain().getMSVCVersion(&D, Args);
----------------
This comment got separated from the the relevant default some time ago. Can you move it to the `return VersionTuple(18)`?


https://reviews.llvm.org/D27477