<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jul 1, 2014 at 8:58 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron.ballman@gmail.com" target="_blank">aaron.ballman@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">On Tue, Jul 1, 2014 at 1:15 AM, Saleem Abdulrasool<br>

<<a href="mailto:compnerd@compnerd.org">compnerd@compnerd.org</a>> wrote:<br>
> Hi rnk, aaron.ballman,<br>
><br>
> This restores the original behaviour of `-fmsc-version`.  The older option<br>
> remains as a mechanism for specifying the basic version information.  A<br>
> secondary option, `-fmsc-version-ex` permits the user to specify an extended<br>
> version to the driver.<br>
><br>
> The new version takes the value as a dot-separated value rather than the<br>
> major * 100 + minor format that `-fmsc-version` format.  This makes it easier to<br>
> specify the value as well as a more flexible manner for specifying the value.<br>
><br>
> Specifying both values is considered an error (TODO: add test case).  The driver<br>
> gives preference to the extended format, defaulting to that format unless the<br>
> old option is specified.<br>
<br>
</div>> Index: include/clang/Basic/LangOptions.def<br>
> ===================================================================<br>
> --- include/clang/Basic/LangOptions.def<br>
> +++ include/clang/Basic/LangOptions.def<br>
> @@ -181,6 +181,7 @@<br>
>          "if non-zero, warn about parameter or return Warn if parameter/return value is larger in bytes than this setting. 0 is no check.")<br>
>  VALUE_LANGOPT(MSCVersion, 32, 0,<br>
>                "version of Microsoft Visual C/C++")<br>
> +VALUE_LANGOPT(MSCVersionEx, 32, 0, "Microsoft Visual C/C++ Extended Version")<br>
<br>
Is there a way we could get away with just one langopt member<br>
variable? I'm not keen on seeing code that has to check both variables<br>
to get the answer.<br></blockquote><div><br></div><div>IMO we should tablegen-ify this, so that we can sort the options by size to achieve better packing.  When we do that, we can use uint64_t as the bitfield type for options larger than 32 bits.  Until then, two 32-bit options is the best we can do.</div>
<div><br></div><div>Besides, do you think anyone will care about the build number in Clang?  Most language changes are introduced on new releases captured in the major version.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

If we do have to keep both variables, I would prefer a better name<br>
than "Ex" (though I do enjoy the joke). Also, if both are needed, can<br>
we normalize the text to be similar between both options?<br>
<br>
>  VALUE_LANGOPT(VtorDispMode, 2, 1, "How many vtordisps to insert")<br>
><br>
>  LANGOPT(ApplePragmaPack, 1, 0, "Apple gcc-compatible #pragma pack handling")<br>
> Index: include/clang/Driver/Options.td<br>
> ===================================================================<br>
> --- include/clang/Driver/Options.td<br>
> +++ include/clang/Driver/Options.td<br>
> @@ -577,6 +577,8 @@<br>
>    HelpText<"Enable full Microsoft Visual C++ compatibility">;<br>
>  def fmsc_version : Joined<["-"], "fmsc-version=">, Group<f_Group>, Flags<[CC1Option, CoreOption]>,<br></blockquote><div><br></div><div>You can remove this as a -cc1 option probably, now that the driver canonicalizes as -fmsc-full-version.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
>    HelpText<"Microsoft compiler version number to report in _MSC_VER (0 = don't define it (default))">;<br>
> +def fmsc_version_ex : Joined<["-"], "fmsc-version-ex=">, Group<f_Group>,<br>
> +    Flags<[CC1Option, CoreOption]>, HelpText<"Extended Microsoft compiler version">;<br>
<br>
I think the public-facing name needs to change to something more<br>
descriptive. How about -fmsc-version-full? Also, I'd like the help<br>
text normalized between the two options.<br></blockquote><div><br></div><div>+1 for -fmsc-full-version.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

>  def fdelayed_template_parsing : Flag<["-"], "fdelayed-template-parsing">, Group<f_Group>,<br>
>    HelpText<"Parse templated function definitions at the end of the "<br>
>             "translation unit">,  Flags<[CC1Option]>;<br>
> Index: lib/Basic/Targets.cpp<br>
> ===================================================================<br>
> --- lib/Basic/Targets.cpp<br>
> +++ lib/Basic/Targets.cpp<br>
> @@ -587,11 +587,13 @@<br>
>      if (Opts.POSIXThreads)<br>
>        Builder.defineMacro("_MT");<br>
><br>
> -    if (Opts.MSCVersion != 0) {<br>
> -      Builder.defineMacro("_MSC_VER", Twine(Opts.MSCVersion / 100000));<br>
> -      Builder.defineMacro("_MSC_FULL_VER", Twine(Opts.MSCVersion));<br>
> +    if (Opts.MSCVersionEx) {<br>
> +      Builder.defineMacro("_MSC_VER", Twine(Opts.MSCVersionEx / 100000));<br>
> +      Builder.defineMacro("_MSC_FULL_VER", Twine(Opts.MSCVersionEx));<br>
>        // FIXME We cannot encode the revision information into 32-bits<br>
>        Builder.defineMacro("_MSC_BUILD", Twine(1));<br>
> +    } else if (Opts.MSCVersion != 0) {<br>
<br>
Can drop the != 0 like you do for the Ex version.<br>
<br>
> +      Builder.defineMacro("_MSC_VER", Twine(Opts.MSCVersion));<br>
<br>
Can we not set the full version as well (just missing the information<br>
we do not have). Same for _MSC_BUILD.<br>
<br>
I think it would be surprising for one of these to exist, but not all three.<br>
<br>
>      }<br>
><br>
>      if (Opts.MicrosoftExt) {<br>
> Index: lib/Driver/Tools.cpp<br>
> ===================================================================<br>
> --- lib/Driver/Tools.cpp<br>
> +++ lib/Driver/Tools.cpp<br>
> @@ -3723,14 +3723,29 @@<br>
>                                                    true))))<br>
>      CmdArgs.push_back("-fms-compatibility");<br>
><br>
> -  // -fmsc-version=1700 is default.<br>
> +  // -fmsc-version-ex=17.00 is default.<br>
>    if (Args.hasFlag(options::OPT_fms_extensions, options::OPT_fno_ms_extensions,<br>
> -                   IsWindowsMSVC) || Args.hasArg(options::OPT_fmsc_version)) {<br>
> -    StringRef msc_ver = Args.getLastArgValue(options::OPT_fmsc_version);<br>
> -    if (msc_ver.empty())<br>
> -      CmdArgs.push_back("-fmsc-version=1700");<br>
> -    else<br>
> -      CmdArgs.push_back(Args.MakeArgString("-fmsc-version=" + msc_ver));<br>
> +                   IsWindowsMSVC) || Args.hasArg(options::OPT_fmsc_version) ||<br>
> +      Args.hasArg(options::OPT_fmsc_version_ex)) {<br>
> +    if (Args.hasArg(options::OPT_fmsc_version) &&<br>
> +        Args.hasArg(options::OPT_fmsc_version_ex))<br>
> +      D.Diag(diag::err_drv_argument_not_allowed_with)<br>
> +          << Args.getArgString(options::OPT_fmsc_version)<br>
> +          << Args.getArgString(options::OPT_fmsc_version_ex);<br>
> +<br>
> +    if (Args.hasArg(options::OPT_fmsc_version_ex)) {<br>
> +      StringRef MSCVerEx = Args.getLastArgValue(options::OPT_fmsc_version_ex); </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

> +      if (MSCVerEx.empty())<br>
> +        CmdArgs.push_back("-fmsc-version-ex=17.00");<br></blockquote><div><br></div><div>This if branch and the other like it is probably unnecessary.</div><div><br></div><div>The logic should be:</div><div>
if (Arg *A = Args.getLastArg(OPT_fmsc_full_version)) { ... }</div><div>else if (Arg *A = Args.getLastArg(OPT_fmsc_version)) { ... }</div><div>else { CmdArgs.push_back("-fmsc-full-version=17.00"); }</div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
> +      else<br>
> +        CmdArgs.push_back(Args.MakeArgString("-fmsc-version-ex=" + MSCVerEx));<br>
> +    } else {<br>
> +      StringRef MSCVer = Args.getLastArgValue(options::OPT_fmsc_version);<br>
> +      if (MSCVer.empty())<br>
> +        CmdArgs.push_back("-fmsc-version-ex=17.00");<br>
> +      else<br>
> +        CmdArgs.push_back(Args.MakeArgString("-fmsc-version=" + MSCVer));<br>
> +    }<br>
>    }<br>
><br>
><br>
> Index: lib/Frontend/CompilerInvocation.cpp<br>
> ===================================================================<br>
> --- lib/Frontend/CompilerInvocation.cpp<br>
> +++ lib/Frontend/CompilerInvocation.cpp<br>
> @@ -1208,7 +1208,7 @@<br>
>  }<br>
><br>
>  static unsigned parseMSCVersion(ArgList &Args, DiagnosticsEngine &Diags) {<br>
> -  auto Arg = Args.getLastArg(OPT_fmsc_version);<br>
> +  auto Arg = Args.getLastArg(OPT_fmsc_version_ex);<br>
>    if (!Arg)<br>
>      return 0;<br>
><br>
> @@ -1225,25 +1225,8 @@<br>
>    // Unfortunately, due to the bit-width limitations, we cannot currently encode<br>
>    // the value for the patch level.<br>
><br>
> -  StringRef Value = Arg->getValue();<br>
> -<br>
> -  // parse the compatible old form of _MSC_VER or the newer _MSC_FULL_VER<br>
> -  if (Value.find('.') == StringRef::npos) {<br>
> -    unsigned Version = 0;<br>
> -    if (Value.getAsInteger(10, Version)) {<br>
> -      Diags.Report(diag::err_drv_invalid_value)<br>
> -        << Arg->getAsString(Args) << Value;<br>
> -      return 0;<br>
> -    }<br>
> -    if (Version < 100)<br>
> -      Version = Version * 100;    // major -> major.minor<br>
> -    if (Version < 100000)<br>
> -      Version = Version * 100000; // major.minor -> major.minor.build<br>
> -    return Version;<br>
> -  }<br>
> -<br>
> -  // parse the dot-delimited component version<br>
>    unsigned VC[4] = {0};<br>
> +  StringRef Value = Arg->getValue();<br>
>    SmallVector<StringRef, 4> Components;<br>
><br>
>    Value.split(Components, ".", llvm::array_lengthof(VC));<br>
> @@ -1430,7 +1413,8 @@<br>
>    Opts.MSVCCompat = Args.hasArg(OPT_fms_compatibility);<br>
>    Opts.MicrosoftExt = Opts.MSVCCompat || Args.hasArg(OPT_fms_extensions);<br>
>    Opts.AsmBlocks = Args.hasArg(OPT_fasm_blocks) || Opts.MicrosoftExt;<br>
> -  Opts.MSCVersion = parseMSCVersion(Args, Diags);<br>
> +  Opts.MSCVersion = getLastArgIntValue(Args, OPT_fmsc_version, 0, Diags);<br>
> +  Opts.MSCVersionEx = parseMSCVersion(Args, Diags);<br>
>    Opts.VtorDispMode = getLastArgIntValue(Args, OPT_vtordisp_mode_EQ, 1, Diags);<br>
>    Opts.Borland = Args.hasArg(OPT_fborland_extensions);<br>
>    Opts.WritableStrings = Args.hasArg(OPT_fwritable_strings);<br>
> Index: lib/Frontend/TextDiagnostic.cpp<br>
> ===================================================================<br>
> --- lib/Frontend/TextDiagnostic.cpp<br>
> +++ lib/Frontend/TextDiagnostic.cpp<br>
> @@ -808,7 +808,8 @@<br>
>        if (DiagOpts->getFormat() == DiagnosticOptions::Msvc) {<br>
>          OS << ',';<br>
>          // Visual Studio 2010 or earlier expects column number to be off by one<br>
> -        if (LangOpts.MSCVersion && LangOpts.MSCVersion < 170000000)<br>
> +        if ((LangOpts.MSCVersion && LangOpts.MSCVersion < 1700) ||<br>
> +            (LangOpts.MSCVersionEx && LangOpts.MSCVersionEx < 170000000))<br>
>            ColNo--;<br>
>        } else<br>
>          OS << ':';<br>
> Index: test/Driver/msc-version.c<br>
> ===================================================================<br>
> --- test/Driver/msc-version.c<br>
> +++ test/Driver/msc-version.c<br>
> @@ -4,37 +4,25 @@<br>
>  // CHECK-NO-MSC-VERSION: _MSC_FULL_VER 170000000<br>
>  // CHECK-NO-MSC-VERSION: _MSC_VER 1700<br>
><br>
> -// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version=1600 -dM -E - </dev/null -o - | FileCheck %s -check-prefix CHECK-MSC-VERSION<br>
> -<br>
> -// CHECK-MSC-VERSION: _MSC_BUILD 1<br>
> -// CHECK-MSC-VERSION: _MSC_FULL_VER 160000000<br>
> -// CHECK-MSC-VERSION: _MSC_VER 1600<br>
> -<br>
> -// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version=160030319 -dM -E - </dev/null -o - | FileCheck %s -check-prefix CHECK-MSC-VERSION-EXT<br>
> -<br>
> -// CHECK-MSC-VERSION-EXT: _MSC_BUILD 1<br>
> -// CHECK-MSC-VERSION-EXT: _MSC_FULL_VER 160030319<br>
> -// CHECK-MSC-VERSION-EXT: _MSC_VER 1600<br>
> -<br>
> -// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version=14 -dM -E - </dev/null -o - | FileCheck %s -check-prefix CHECK-MSC-VERSION-MAJOR<br>
> +// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version-ex=14 -dM -E - </dev/null -o - | FileCheck %s -check-prefix CHECK-MSC-VERSION-MAJOR<br>
><br>
>  // CHECK-MSC-VERSION-MAJOR: _MSC_BUILD 1<br>
>  // CHECK-MSC-VERSION-MAJOR: _MSC_FULL_VER 140000000<br>
>  // CHECK-MSC-VERSION-MAJOR: _MSC_VER 1400<br>
><br>
> -// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version=17.00 -dM -E - </dev/null -o - | FileCheck %s -check-prefix CHECK-MSC-VERSION-MAJOR-MINOR<br>
> +// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version-ex=15.00 -dM -E - </dev/null -o - | FileCheck %s -check-prefix CHECK-MSC-VERSION-MAJOR-MINOR<br>
><br>
>  // CHECK-MSC-VERSION-MAJOR-MINOR: _MSC_BUILD 1<br>
> -// CHECK-MSC-VERSION-MAJOR-MINOR: _MSC_FULL_VER 170000000<br>
> -// CHECK-MSC-VERSION-MAJOR-MINOR: _MSC_VER 1700<br>
> +// CHECK-MSC-VERSION-MAJOR-MINOR: _MSC_FULL_VER 150000000<br>
> +// CHECK-MSC-VERSION-MAJOR-MINOR: _MSC_VER 1500<br>
><br>
> -// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version=15.00.20706 -dM -E - </dev/null -o - | FileCheck %s -check-prefix CHECK-MSC-VERSION-MAJOR-MINOR-BUILD<br>
> +// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version-ex=15.00.20706 -dM -E - </dev/null -o - | FileCheck %s -check-prefix CHECK-MSC-VERSION-MAJOR-MINOR-BUILD<br>
><br>
>  // CHECK-MSC-VERSION-MAJOR-MINOR-BUILD: _MSC_BUILD 1<br>
>  // CHECK-MSC-VERSION-MAJOR-MINOR-BUILD: _MSC_FULL_VER 150020706<br>
>  // CHECK-MSC-VERSION-MAJOR-MINOR-BUILD: _MSC_VER 1500<br>
><br>
> -// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version=15.00.20706.01 -dM -E - </dev/null -o - | FileCheck %s -check-prefix CHECK-MSC-VERSION-MAJOR-MINOR-BUILD-PATCH<br>
> +// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version-ex=15.00.20706.01 -dM -E - </dev/null -o - | FileCheck %s -check-prefix CHECK-MSC-VERSION-MAJOR-MINOR-BUILD-PATCH<br>
><br>
>  // CHECK-MSC-VERSION-MAJOR-MINOR-BUILD-PATCH: _MSC_BUILD 1<br>
>  // CHECK-MSC-VERSION-MAJOR-MINOR-BUILD-PATCH: _MSC_FULL_VER 150020706<br>
> Index: test/Misc/diag-format.c<br>
> ===================================================================<br>
> --- test/Misc/diag-format.c<br>
> +++ test/Misc/diag-format.c<br>
> @@ -3,10 +3,13 @@<br>
>  // RUN: %clang -fsyntax-only -fdiagnostics-format=clang -target x86_64-pc-win32 %s 2>&1 | FileCheck %s -check-prefix=DEFAULT<br>
>  //<br>
>  // RUN: %clang -fsyntax-only -fdiagnostics-format=msvc -fmsc-version=1300  %s 2>&1 | FileCheck %s -check-prefix=MSVC2010<br>
> +// RUN: %clang -fsyntax-only -fdiagnostics-format=msvc -fmsc-version-ex=13.00  %s 2>&1 | FileCheck %s -check-prefix=MSVC2010<br>
>  // RUN: %clang -fsyntax-only -fdiagnostics-format=msvc  %s 2>&1 | FileCheck %s -check-prefix=MSVC<br>
>  // RUN: %clang -fsyntax-only -fdiagnostics-format=msvc -fmsc-version=1300 -target x86_64-pc-win32 %s 2>&1 | FileCheck %s -check-prefix=MSVC2010<br>
> +// RUN: %clang -fsyntax-only -fdiagnostics-format=msvc -fmsc-version-ex=13.00 -target x86_64-pc-win32 %s 2>&1 | FileCheck %s -check-prefix=MSVC2010<br>
>  // RUN: %clang -fsyntax-only -fdiagnostics-format=msvc -target x86_64-pc-win32 %s 2>&1 | FileCheck %s -check-prefix=MSVC<br>
>  // RUN: %clang -fsyntax-only -fdiagnostics-format=msvc -fmsc-version=1300 -target x86_64-pc-win32 -fshow-column %s 2>&1 | FileCheck %s -check-prefix=MSVC2010<br>
> +// RUN: %clang -fsyntax-only -fdiagnostics-format=msvc -fmsc-version-ex=13.00 -target x86_64-pc-win32 -fshow-column %s 2>&1 | FileCheck %s -check-prefix=MSVC2010<br>
>  // RUN: %clang -fsyntax-only -fdiagnostics-format=msvc -target x86_64-pc-win32 -fshow-column %s 2>&1 | FileCheck %s -check-prefix=MSVC<br>
>  //<br>
>  // RUN: %clang -fsyntax-only -fdiagnostics-format=vi    %s 2>&1 | FileCheck %s -check-prefix=VI<br>
> @@ -16,6 +19,7 @@<br>
>  // RUN: %clang -fsyntax-only -fno-show-column %s 2>&1 | FileCheck %s -check-prefix=NO_COLUMN<br>
>  //<br>
>  // RUN: not %clang -fsyntax-only -Werror -fdiagnostics-format=msvc-fallback -fmsc-version=1300 %s 2>&1 | FileCheck %s -check-prefix=MSVC2010-FALLBACK<br>
> +// RUN: not %clang -fsyntax-only -Werror -fdiagnostics-format=msvc-fallback -fmsc-version-ex=13.00 %s 2>&1 | FileCheck %s -check-prefix=MSVC2010-FALLBACK<br>
>  // RUN: not %clang -fsyntax-only -Werror -fdiagnostics-format=msvc-fallback %s 2>&1 | FileCheck %s -check-prefix=MSVC-FALLBACK<br>
><br>
><br>
> @@ -30,12 +34,12 @@<br>
><br>
>  #ifdef foo<br>
>  #endif bad // extension!<br>
> -// DEFAULT: {{.*}}:32:8: warning: extra tokens at end of #endif directive [-Wextra-tokens]<br>
> -// MSVC2010: {{.*}}(32,7) : warning: extra tokens at end of #endif directive [-Wextra-tokens]<br>
> -// MSVC: {{.*}}(32,8) : warning: extra tokens at end of #endif directive [-Wextra-tokens]<br>
> -// VI: {{.*}} +32:8: warning: extra tokens at end of #endif directive [-Wextra-tokens]<br>
> -// MSVC_ORIG: {{.*}}(32) : warning: extra tokens at end of #endif directive [-Wextra-tokens]<br>
> -// NO_COLUMN: {{.*}}:32: warning: extra tokens at end of #endif directive [-Wextra-tokens]<br>
> -// MSVC2010-FALLBACK: {{.*}}(32,7) : error(clang): extra tokens at end of #endif directive<br>
> -// MSVC-FALLBACK: {{.*}}(32,8) : error(clang): extra tokens at end of #endif directive<br>
> +// DEFAULT: {{.*}}:36:8: warning: extra tokens at end of #endif directive [-Wextra-tokens]<br>
> +// MSVC2010: {{.*}}(36,7) : warning: extra tokens at end of #endif directive [-Wextra-tokens]<br>
> +// MSVC: {{.*}}(36,8) : warning: extra tokens at end of #endif directive [-Wextra-tokens]<br>
> +// VI: {{.*}} +36:8: warning: extra tokens at end of #endif directive [-Wextra-tokens]<br>
> +// MSVC_ORIG: {{.*}}(36) : warning: extra tokens at end of #endif directive [-Wextra-tokens]<br>
> +// NO_COLUMN: {{.*}}:36: warning: extra tokens at end of #endif directive [-Wextra-tokens]<br>
> +// MSVC2010-FALLBACK: {{.*}}(36,7) : error(clang): extra tokens at end of #endif directive<br>
> +// MSVC-FALLBACK: {{.*}}(36,8) : error(clang): extra tokens at end of #endif directive<br>
>  int x;<br>
><br>
<span class=""><font color="#888888"><br>
~Aaron<br>
</font></span></blockquote></div><br></div></div>