[PATCH] clang-cl: add support for /? and /help
Reid Kleckner
rnk at google.com
Thu Jul 25 10:03:42 PDT 2013
Re: cmake+ninja and generated headers, it's a CMake bug:
http://public.kitware.com/Bug/view.php?id=14167
The issue has a patch that I use locally to solve the problem. Without the
patch, you have to run ninja at most twice (no more) to make sure
everything is up-to-date. Very annoying.
On Thu, Jul 25, 2013 at 9:46 AM, Hans Wennborg <hans at chromium.org> wrote:
>
> > 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 @@
> Group<f_Group>;
> -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>">,
> + HelpText<"CL.EXE COMPATIBILITY 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.
>
>
> http://llvm-reviews.chandlerc.com/D1215
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130725/3f40a84a/attachment.html>
More information about the cfe-commits
mailing list