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.
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.
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.
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.
Much of this is make-work, and sorry, but unless it's going to significantly
slow you down I think it is important.
Also, *please* don't do it all in this patch. =D I would propose a set of
patches as follows:
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.
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?)
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.
4) This patch which adds CLCompatOptions.td and leverages the now
rationalized groups.
5) Any general cleanups to the options stuff that I ask for in passing (sorry
to ask for these in passing, but always feel free to defer to a later patch).
While this is somewhat more work, I'm hoping it won't be slow work. I can say
that the patches will be very fast for me to review. Hopefully some of these
steps can be parallelized/pipelined as well.
I have a bunch of detailed comments inline, but defer to my high level
comment here if those inline comments don't make sense.
================
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)
};
----------------
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.
================
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;
+
/////////
----------------
Especially here I think it would be useful to reorder things:
core clang options, gcc compat options, cl compat options, cc1 internal
options, ...
================
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,
----------------
Not in this patch, but please go through and nuke the CCC prefix from these
classes. They no longer make any sense.
================
Comment at: include/clang/Driver/Options.td:135
@@ -127,3 +134,3 @@
-class CCCDebugOpt : Group<ccc_debug_Group>, Flags<[DriverOption, HelpHidden]>;
+class CCCDebugOpt : Group<ccc_debug_Group>, Flags<[DriverOption, HelpHidden,
ClangOption]>;
def ccc_install_dir : Separate<["-"], "ccc-install-dir">, CCCDebugOpt,
----------------
Ditto.
================
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]>,
----------------
I would only really support the joined form of -Xclang= rather than the
separated form... but your call.
================
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>;
----------------
I would ask to fix 80-column violations and maintain an 80-column limit on the
lines you change.
================
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>,
----------------
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.
================
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]>,
----------------
You should rename this to -fdiagnostics-error-limit and leave this as a
compatibility alias IMO. Separate patch of course.
================
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>;
----------------
This should also be canonically spelled -fdiagnostics-spell-checking IMO.
================
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]>,
----------------
This should be canonically spelled '--integrated-as' to fit with other
driver-level options.
================
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">;
+
----------------
I would put this in a CLCompatOptions.td file and include it.
================
Comment at: lib/Driver/Driver.cpp:622
@@ -609,4 +621,3 @@
void Driver::PrintHelp(bool ShowHidden) const {
- getOpts().PrintHelp(
- llvm::outs(), Name.c_str(), DriverTitle.c_str(), /*Include*/ 0,
- /*Exclude*/ options::NoDriverOption | (ShowHidden ? 0 : HelpHidden));
+ unsigned FlagsToInclude = 0;
+ unsigned FlagsToExclude = options::NoDriverOption;
----------------
Add an enum for this state?
================
Comment at: lib/Driver/Driver.cpp:625-633
@@ +624,11 @@
+
+ if (!ShowHidden)
+ FlagsToExclude |= HelpHidden;
+
+ if (Mode == CLMode) {
+ // In CL mode, only allow core Clang options and CL options.
+ FlagsToInclude = options::ClangOption | options::CLOption;
+ } else {
+ FlagsToExclude |= options::CLOption;
+ }
+
----------------
It seems unfortunate to duplicate this from above Is there no way to share it?
http://llvm-reviews.chandlerc.com/D1215
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
