[PATCH] clang-cl: add support for /? and /help

Hans Wennborg hans at chromium.org
Thu Jul 25 09:46:22 PDT 2013

  > So, the description of this patch is somewhat misleading. This patch is about creating the fundamental option grouping. Awesome. Not sure how much you can clarify in Phab, but make sure that's clear in the eventual commit log.

  I'll make sure the commit message is more clear.

  > Can you add some documentation somewhere about the scheme you're proposing? You can base it somewhat on my email about the driver option categorization, somewhat on rnk's comments. It's all in the same direction. I would at least put this at the top of Options.td so folks understand what is expected going forward.

  Will do.

  > Second, a lot of my comments below are along the lines of "fix the grouping of options we have to be rational, and then use that to do the CL-opt-in". I think this is really important to make sure we get it right. I suggest doing it now because I think it'll be fairly easy to do. If something explodes into a lot of work, push back! =] I'm happy to convert really hard stuff here to FIXMEs as long as you eventually fix them.

  I think this is the big ticket item here. I'm not sure if by grouping you mean changing the Group (f_Group, etc.) on the options, or you mean how they are actually ordered in the file, or both.

  I think the Group classes might be orthogonal to what we're trying to do here. They're good for categorizing whether something is a debug or a warning related option, etc., so that such options can be handled in one place, but I don't think they would work for excluding or including options in cl-mode vs gcc-mode. An option can only be in one Group (which can, I think, be part of a larger Group), so we couldn't have options be part of both a group_Debug and group_CL.

  Flags are good for that, because they can cut across groups.

  I think reordering the options into "clang options, gcc compat options, cl compat options, cc1 internal options" like you suggest in the comment below sounds great. We should then be able to put flags on the options in a straight-forward way (CLCompat, GCCComap, CC1Option) based on that ordering.

  > Third, I've left a bunch of comments along the lines of "this option is spelled incorrectly". The reason is this: we need to start splitting the way we think of options into three notional buckets: options we *want* to support, options we *have* to support for compatibility but have a preferred spelling (or prefer to avoid), and options that we consider legacy and will *eventually* remove (where eventually may be on the order of years or decades, *not soon*). Once we start thinking this way, it seems to me we should make sure that only the first and second buckets make it into the new interface we're defining for a Clang-based CL-compatible driver. To that end, many of the existing Clang options that we want to re-export in the CL mode I first want to give the right canonical name, put the old in the deprecated bucket and only re-export the canonical name.

  Sure, this sounds fine to me. The plan is to start with the CL mode bucket almost empty, so I guess we can then do the renames as we go along and add things?

  > 1) A patch which creates a more rational set of f_Group subdivisions that we can use and updates the various diagnostics to have the right group.
  > 1a) One patch for each of the other *_Group sets which needs subdivision similar to f_Group -- not sure there are any though.

  As I said above, this is the part I'm unsure about. I'm not sure we want to be using the Groups for this.

  > 2) A patch which creates new files and moves a few *obvious* flags into them:
  > DeprecatedOptions.td (start with a few of the -ccc flags we have better spellings for?)
  > GCCCompatOptions.td (only move flags initially which are *never used* by Clang?)

  I'll have to figure out how to make the CMake build track include dependencies for .td files; I'm not convinced that's currently working :)

  > 3) One patch per flag you have asked to put into the CL interface here and I have asked for a rename. Fix everything to do with that flag.

  Sounds good.

Comment at: include/clang/Driver/Options.h:32-34
@@ -31,3 +31,5 @@
   CC1Option = (1 << 9),
-  NoDriverOption = (1 << 10)
+  NoDriverOption = (1 << 10),
+  ClangOption = (1 << 11),
+  CLOption = (1 << 12)
Chandler Carruth wrote:
> Why order these in this way? Are these values things that must be stable for all time?
> If so, omg, why, ok. add some comments.
> If not, I would keep NoDriverOption at the end.
The order is not important. I just added to the end, but I agree that keeping NoDriverOption last might look better.

Comment at: include/clang/Driver/Options.td:45-51
@@ -44,2 +44,9 @@
+// ClangOption - This is a core clang option, available in both gcc and
+// cl.exe compatibility modes.
+def ClangOption : OptionFlag;
+// CLOption - This is a cl.exe compatibility option.
+def CLOption : OptionFlag;
Chandler Carruth wrote:
> Especially here I think it would be useful to reorder things:
> core clang options, gcc compat options, cl compat options, cc1 internal options, ...
I agree.

Comment at: include/clang/Driver/Options.td:121
@@ -113,3 +120,3 @@
-class CCCDriverOpt : Group<ccc_driver_Group>, Flags<[DriverOption, HelpHidden]>;
+class CCCDriverOpt : Group<ccc_driver_Group>, Flags<[DriverOption, HelpHidden, ClangOption]>;
 def driver_mode : Joined<["--"], "driver-mode=">, CCCDriverOpt,
Chandler Carruth wrote:
> Not in this patch, but please go through and nuke the CCC prefix from these classes. They no longer make any sense.
Okay, will do in separate patch.

Comment at: include/clang/Driver/Options.td:260-262
@@ -252,5 +259,5 @@
   HelpText<"Pass <arg> to the assembler">, MetaVarName<"<arg>">;
 def Xclang : Separate<["-"], "Xclang">,
   HelpText<"Pass <arg> to the clang compiler">, MetaVarName<"<arg>">,
-  Flags<[NoForward]>;
+  Flags<[NoForward, ClangOption]>;
 def Xlinker : Separate<["-"], "Xlinker">, Flags<[LinkerInput, RenderAsInput]>,
Chandler Carruth wrote:
> I would only really support the joined form of -Xclang= rather than the separated form... but your call.
I'll look at doing this in a separate patch.

Comment at: include/clang/Driver/Options.td:384
@@ -376,3 +383,3 @@
-def fno_crash_diagnostics : Flag<["-"], "fno-crash-diagnostics">, Group<f_clang_Group>, Flags<[NoArgumentUnused]>;
+def fno_crash_diagnostics : Flag<["-"], "fno-crash-diagnostics">, Group<f_clang_Group>, Flags<[NoArgumentUnused, ClangOption]>;
 def fcreate_profile : Flag<["-"], "fcreate-profile">, Group<f_Group>;
Chandler Carruth wrote:
> I would ask to fix 80-column violations and maintain an 80-column limit on the lines you change.
Will do.

Comment at: include/clang/Driver/Options.td:391-407
@@ -383,19 +390,19 @@
 def fdebug_pass_structure : Flag<["-"], "fdebug-pass-structure">, Group<f_Group>;
-def fdiagnostics_fixit_info : Flag<["-"], "fdiagnostics-fixit-info">, Group<f_clang_Group>;
+def fdiagnostics_fixit_info : Flag<["-"], "fdiagnostics-fixit-info">, Flags<[ClangOption]>, Group<f_clang_Group>;
 def fdiagnostics_parseable_fixits : Flag<["-"], "fdiagnostics-parseable-fixits">, Group<f_clang_Group>,
-    Flags<[CC1Option]>, HelpText<"Print fix-its in machine parseable form">;
+    Flags<[CC1Option, ClangOption]>, HelpText<"Print fix-its in machine parseable form">;
 def fdiagnostics_print_source_range_info : Flag<["-"], "fdiagnostics-print-source-range-info">,
-    Group<f_clang_Group>,  Flags<[CC1Option]>,
+    Group<f_clang_Group>,  Flags<[CC1Option, ClangOption]>,
     HelpText<"Print source range spans in numeric form">;
 def fdiagnostics_show_option : Flag<["-"], "fdiagnostics-show-option">, Group<f_Group>,
-    Flags<[CC1Option]>, HelpText<"Print option name with mappable diagnostics">;
+    Flags<[CC1Option, ClangOption]>, HelpText<"Print option name with mappable diagnostics">;
 def fdiagnostics_show_name : Flag<["-"], "fdiagnostics-show-name">, Group<f_Group>,
-    Flags<[CC1Option]>, HelpText<"Print diagnostic name">;
+    Flags<[CC1Option, ClangOption]>, HelpText<"Print diagnostic name">;
 def fdiagnostics_show_note_include_stack : Flag<["-"], "fdiagnostics-show-note-include-stack">,
-    Group<f_Group>,  Flags<[CC1Option]>, HelpText<"Display include stacks for diagnostic notes">;
-def fdiagnostics_format_EQ : Joined<["-"], "fdiagnostics-format=">, Group<f_clang_Group>;
-def fdiagnostics_show_category_EQ : Joined<["-"], "fdiagnostics-show-category=">, Group<f_clang_Group>;
+    Group<f_Group>,  Flags<[CC1Option, ClangOption]>, HelpText<"Display include stacks for diagnostic notes">;
+def fdiagnostics_format_EQ : Joined<["-"], "fdiagnostics-format=">, Flags<[ClangOption]>, Group<f_clang_Group>;
+def fdiagnostics_show_category_EQ : Joined<["-"], "fdiagnostics-show-category=">, Flags<[ClangOption]>, Group<f_clang_Group>;
 def fdiagnostics_show_template_tree : Flag<["-"], "fdiagnostics-show-template-tree">,
-    Group<f_Group>, Flags<[CC1Option]>,
+    Group<f_Group>, Flags<[CC1Option, ClangOption]>,
     HelpText<"Print a template comparison tree for differing templates">;
 def fdollars_in_identifiers : Flag<["-"], "fdollars-in-identifiers">, Group<f_Group>,
Chandler Carruth wrote:
> This clearly isn't the right approach.
> We don't want to auto-include everything in the f_Group and f_clang_Group, but what we can do is split all of the things we *don't* want into their own f_foo_Group. Specifically, I think we should have an f_gcc_compat_Group and f_apple_Group at the very least for the flags that are GCC-compatibility spellings and Apple-specific extensions (-fapple-kext, dunno if there are others). There are probably other rational subdivisions of the f_Group. The way to whitelist the right subset of the current f_Group is to create such a subgroup and then do it in the abstract way.
As I said in my main comment, I'm not sure if including/excluding things in the different driver modes based on the Group is the right way.

Comment at: include/clang/Driver/Options.td:422
@@ -414,3 +421,3 @@
 def fencoding_EQ : Joined<["-"], "fencoding=">, Group<f_Group>;
-def ferror_limit_EQ : Joined<["-"], "ferror-limit=">, Group<f_Group>;
+def ferror_limit_EQ : Joined<["-"], "ferror-limit=">, Group<f_Group>, Flags<[ClangOption]>;
 def fexceptions : Flag<["-"], "fexceptions">, Group<f_Group>, Flags<[CC1Option]>,
Chandler Carruth wrote:
> You should rename this to -fdiagnostics-error-limit and leave this as a compatibility alias IMO. Separate patch of course.
I started looking at this, but then had second thoughts. I don't think -ferror-limit= is a bad name, and I'm not sure where we want to go with this. Prepending -fdiagnostics- makes it longer. It might be fine for this option, but do we want to do this for the other diagnostics related options too? -fdiagnostics-template-bakctrace-limit? Should we be consistent and canonicalize -fdiagnostics-color?

Comment at: include/clang/Driver/Options.td:723
@@ -715,3 +722,3 @@
 def fshow_source_location : Flag<["-"], "fshow-source-location">, Group<f_Group>;
-def fspell_checking : Flag<["-"], "fspell-checking">, Group<f_Group>;
+def fspell_checking : Flag<["-"], "fspell-checking">, Flags<[ClangOption]>, Group<f_Group>;
 def fsigned_bitfields : Flag<["-"], "fsigned-bitfields">, Group<f_Group>;
Chandler Carruth wrote:
> This should also be canonically spelled -fdiagnostics-spell-checking IMO.
This sounds good to me because it makes it less ambiguous what kind of spell checking it refers to.

Comment at: include/clang/Driver/Options.td:859
@@ -851,3 +858,3 @@
 def install__name : Separate<["-"], "install_name">;
-def integrated_as : Flag<["-"], "integrated-as">, Flags<[DriverOption]>;
+def integrated_as : Flag<["-"], "integrated-as">, Flags<[DriverOption, ClangOption]>;
 def iprefix : JoinedOrSeparate<["-"], "iprefix">, Group<clang_i_Group>, Flags<[CC1Option]>,
Chandler Carruth wrote:
> This should be canonically spelled '--integrated-as' to fit with other driver-level options.
Will do.

Comment at: include/clang/Driver/Options.td:1320-1322
@@ +1319,5 @@
+// clang-cl options.
+def cl_Group : OptionGroup<"<clang-cl options>">,
Chandler Carruth wrote:
> I would put this in a CLCompatOptions.td file and include it.
Right. Just have to figure out the dependency tracking.


More information about the cfe-commits mailing list