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 <[email protected]> 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 >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
