<div dir="ltr">Re: cmake+ninja and generated headers, it's a CMake bug: <a href="http://public.kitware.com/Bug/view.php?id=14167">http://public.kitware.com/Bug/view.php?id=14167</a><div><br></div><div>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.</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jul 25, 2013 at 9:46 AM, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im"><br>
> 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.<br>
<br>
</div> I'll make sure the commit message is more clear.<br>
<div class="im"><br>
> 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.<br>
<br>
</div> Will do.<br>
<div class="im"><br>
> 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.<br>
<br>
</div> 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.<br>
<br>
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.<br>
<br>
Flags are good for that, because they can cut across groups.<br>
<br>
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.<br>
<div class="im"><br>
<br>
> 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.<br>
<br>
</div> 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?<br>
<div class="im"><br>
> 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.<br>
><br>
> 1a) One patch for each of the other *_Group sets which needs subdivision similar to f_Group -- not sure there are any though.<br>
<br>
</div> 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.<br>
<div class="im"><br>
> 2) A patch which creates new files and moves a few *obvious* flags into them:<br>
><br>
</div><div class="im"> > DeprecatedOptions.td (start with a few of the -ccc flags we have better spellings for?)<br>
</div><div class="im"> > GCCCompatOptions.td (only move flags initially which are *never used* by Clang?)<br>
<br>
</div> 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 :)<br>
<div class="im"><br>
> 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.<br>
<br>
</div> Sounds good.<br>
<div class="im"><br>
<br>
================<br>
Comment at: include/clang/Driver/Options.h:32-34<br>
@@ -31,3 +31,5 @@<br>
CC1Option = (1 << 9),<br>
- NoDriverOption = (1 << 10)<br>
+ NoDriverOption = (1 << 10),<br>
+ ClangOption = (1 << 11),<br>
+ CLOption = (1 << 12)<br>
};<br>
----------------<br>
</div><div class="im">Chandler Carruth wrote:<br>
> Why order these in this way? Are these values things that must be stable for all time?<br>
><br>
> If so, omg, why, ok. add some comments.<br>
><br>
> If not, I would keep NoDriverOption at the end.<br>
</div>The order is not important. I just added to the end, but I agree that keeping NoDriverOption last might look better.<br>
<div class="im"><br>
================<br>
Comment at: include/clang/Driver/Options.td:45-51<br>
@@ -44,2 +44,9 @@<br>
<br>
+// ClangOption - This is a core clang option, available in both gcc and<br>
+// cl.exe compatibility modes.<br>
+def ClangOption : OptionFlag;<br>
+<br>
+// CLOption - This is a cl.exe compatibility option.<br>
+def CLOption : OptionFlag;<br>
+<br>
/////////<br>
----------------<br>
</div><div class="im">Chandler Carruth wrote:<br>
> Especially here I think it would be useful to reorder things:<br>
><br>
> core clang options, gcc compat options, cl compat options, cc1 internal options, ...<br>
</div>I agree.<br>
<div class="im"><br>
================<br>
Comment at: include/clang/Driver/Options.td:121<br>
@@ -113,3 +120,3 @@<br>
<br>
-class CCCDriverOpt : Group<ccc_driver_Group>, Flags<[DriverOption, HelpHidden]>;<br>
+class CCCDriverOpt : Group<ccc_driver_Group>, Flags<[DriverOption, HelpHidden, ClangOption]>;<br>
def driver_mode : Joined<["--"], "driver-mode=">, CCCDriverOpt,<br>
----------------<br>
</div><div class="im">Chandler Carruth wrote:<br>
> Not in this patch, but please go through and nuke the CCC prefix from these classes. They no longer make any sense.<br>
</div>Okay, will do in separate patch.<br>
<div class="im"><br>
================<br>
Comment at: include/clang/Driver/Options.td:260-262<br>
@@ -252,5 +259,5 @@<br>
HelpText<"Pass <arg> to the assembler">, MetaVarName<"<arg>">;<br>
def Xclang : Separate<["-"], "Xclang">,<br>
HelpText<"Pass <arg> to the clang compiler">, MetaVarName<"<arg>">,<br>
- Flags<[NoForward]>;<br>
+ Flags<[NoForward, ClangOption]>;<br>
def Xlinker : Separate<["-"], "Xlinker">, Flags<[LinkerInput, RenderAsInput]>,<br>
----------------<br>
</div><div class="im">Chandler Carruth wrote:<br>
> I would only really support the joined form of -Xclang= rather than the separated form... but your call.<br>
</div>I'll look at doing this in a separate patch.<br>
<div class="im"><br>
================<br>
Comment at: include/clang/Driver/Options.td:384<br>
@@ -376,3 +383,3 @@<br>
Group<f_Group>;<br>
-def fno_crash_diagnostics : Flag<["-"], "fno-crash-diagnostics">, Group<f_clang_Group>, Flags<[NoArgumentUnused]>;<br>
+def fno_crash_diagnostics : Flag<["-"], "fno-crash-diagnostics">, Group<f_clang_Group>, Flags<[NoArgumentUnused, ClangOption]>;<br>
def fcreate_profile : Flag<["-"], "fcreate-profile">, Group<f_Group>;<br>
----------------<br>
</div><div class="im">Chandler Carruth wrote:<br>
> I would ask to fix 80-column violations and maintain an 80-column limit on the lines you change.<br>
</div>Will do.<br>
<div><div class="h5"><br>
================<br>
Comment at: include/clang/Driver/Options.td:391-407<br>
@@ -383,19 +390,19 @@<br>
def fdebug_pass_structure : Flag<["-"], "fdebug-pass-structure">, Group<f_Group>;<br>
-def fdiagnostics_fixit_info : Flag<["-"], "fdiagnostics-fixit-info">, Group<f_clang_Group>;<br>
+def fdiagnostics_fixit_info : Flag<["-"], "fdiagnostics-fixit-info">, Flags<[ClangOption]>, Group<f_clang_Group>;<br>
def fdiagnostics_parseable_fixits : Flag<["-"], "fdiagnostics-parseable-fixits">, Group<f_clang_Group>,<br>
- Flags<[CC1Option]>, HelpText<"Print fix-its in machine parseable form">;<br>
+ Flags<[CC1Option, ClangOption]>, HelpText<"Print fix-its in machine parseable form">;<br>
def fdiagnostics_print_source_range_info : Flag<["-"], "fdiagnostics-print-source-range-info">,<br>
- Group<f_clang_Group>, Flags<[CC1Option]>,<br>
+ Group<f_clang_Group>, Flags<[CC1Option, ClangOption]>,<br>
HelpText<"Print source range spans in numeric form">;<br>
def fdiagnostics_show_option : Flag<["-"], "fdiagnostics-show-option">, Group<f_Group>,<br>
- Flags<[CC1Option]>, HelpText<"Print option name with mappable diagnostics">;<br>
+ Flags<[CC1Option, ClangOption]>, HelpText<"Print option name with mappable diagnostics">;<br>
def fdiagnostics_show_name : Flag<["-"], "fdiagnostics-show-name">, Group<f_Group>,<br>
- Flags<[CC1Option]>, HelpText<"Print diagnostic name">;<br>
+ Flags<[CC1Option, ClangOption]>, HelpText<"Print diagnostic name">;<br>
def fdiagnostics_show_note_include_stack : Flag<["-"], "fdiagnostics-show-note-include-stack">,<br>
- Group<f_Group>, Flags<[CC1Option]>, HelpText<"Display include stacks for diagnostic notes">;<br>
-def fdiagnostics_format_EQ : Joined<["-"], "fdiagnostics-format=">, Group<f_clang_Group>;<br>
-def fdiagnostics_show_category_EQ : Joined<["-"], "fdiagnostics-show-category=">, Group<f_clang_Group>;<br>
+ Group<f_Group>, Flags<[CC1Option, ClangOption]>, HelpText<"Display include stacks for diagnostic notes">;<br>
+def fdiagnostics_format_EQ : Joined<["-"], "fdiagnostics-format=">, Flags<[ClangOption]>, Group<f_clang_Group>;<br>
+def fdiagnostics_show_category_EQ : Joined<["-"], "fdiagnostics-show-category=">, Flags<[ClangOption]>, Group<f_clang_Group>;<br>
def fdiagnostics_show_template_tree : Flag<["-"], "fdiagnostics-show-template-tree">,<br>
- Group<f_Group>, Flags<[CC1Option]>,<br>
+ Group<f_Group>, Flags<[CC1Option, ClangOption]>,<br>
HelpText<"Print a template comparison tree for differing templates">;<br>
def fdollars_in_identifiers : Flag<["-"], "fdollars-in-identifiers">, Group<f_Group>,<br>
----------------<br>
</div></div><div class="im">Chandler Carruth wrote:<br>
> This clearly isn't the right approach.<br>
><br>
> 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.<br>
</div>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.<br>
<div class="im"><br>
================<br>
Comment at: include/clang/Driver/Options.td:422<br>
@@ -414,3 +421,3 @@<br>
def fencoding_EQ : Joined<["-"], "fencoding=">, Group<f_Group>;<br>
-def ferror_limit_EQ : Joined<["-"], "ferror-limit=">, Group<f_Group>;<br>
+def ferror_limit_EQ : Joined<["-"], "ferror-limit=">, Group<f_Group>, Flags<[ClangOption]>;<br>
def fexceptions : Flag<["-"], "fexceptions">, Group<f_Group>, Flags<[CC1Option]>,<br>
----------------<br>
</div><div class="im">Chandler Carruth wrote:<br>
> You should rename this to -fdiagnostics-error-limit and leave this as a compatibility alias IMO. Separate patch of course.<br>
</div>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?<br>
<div class="im"><br>
================<br>
Comment at: include/clang/Driver/Options.td:723<br>
@@ -715,3 +722,3 @@<br>
def fshow_source_location : Flag<["-"], "fshow-source-location">, Group<f_Group>;<br>
-def fspell_checking : Flag<["-"], "fspell-checking">, Group<f_Group>;<br>
+def fspell_checking : Flag<["-"], "fspell-checking">, Flags<[ClangOption]>, Group<f_Group>;<br>
def fsigned_bitfields : Flag<["-"], "fsigned-bitfields">, Group<f_Group>;<br>
----------------<br>
</div><div class="im">Chandler Carruth wrote:<br>
> This should also be canonically spelled -fdiagnostics-spell-checking IMO.<br>
</div>This sounds good to me because it makes it less ambiguous what kind of spell checking it refers to.<br>
<div class="im"><br>
================<br>
Comment at: include/clang/Driver/Options.td:859<br>
@@ -851,3 +858,3 @@<br>
def install__name : Separate<["-"], "install_name">;<br>
-def integrated_as : Flag<["-"], "integrated-as">, Flags<[DriverOption]>;<br>
+def integrated_as : Flag<["-"], "integrated-as">, Flags<[DriverOption, ClangOption]>;<br>
def iprefix : JoinedOrSeparate<["-"], "iprefix">, Group<clang_i_Group>, Flags<[CC1Option]>,<br>
----------------<br>
</div><div class="im">Chandler Carruth wrote:<br>
> This should be canonically spelled '--integrated-as' to fit with other driver-level options.<br>
</div>Will do.<br>
<div class="im"><br>
================<br>
Comment at: include/clang/Driver/Options.td:1320-1322<br>
@@ +1319,5 @@<br>
+<br>
+// clang-cl options.<br>
+def cl_Group : OptionGroup<"<clang-cl options>">,<br>
+ HelpText<"CL.EXE COMPATIBILITY OPTIONS">;<br>
+<br>
----------------<br>
</div><div class="im">Chandler Carruth wrote:<br>
> I would put this in a CLCompatOptions.td file and include it.<br>
</div>Right. Just have to figure out the dependency tracking.<br>
<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D1215" target="_blank">http://llvm-reviews.chandlerc.com/D1215</a><br>
</blockquote></div><br></div>