LGTM Sorry, this fell out of my inbox.
On Tue, Jul 16, 2013 at 1:50 PM, Hans Wennborg <[email protected]> wrote: > Thanks for the review! > > On Tue, Jul 16, 2013 at 6:42 AM, Reid Kleckner <[email protected]> wrote: > >> +def ccc_mode : Joined<["-"], "ccc-mode=">, CCCDriverOpt, > >> + HelpText<"Set the ccc mode to either 'gcc', 'g++' or 'cpp'">; > > > > -ccc creates a nice namespace for clang, but I don't think it's at all > > intuitive and it's now a misnomer (clang "c" compiler) and I'm not sure > if > > we want to promote it. > > > > There was also some discussion about preferring "--" prefixes for new > long > > options, so I'd go with that. > > > > lld calls this the driver "flavor" rather than mode. It would be nice to > > stay consistent on terminology, if folks are OK with flavor over mode. > > > > Putting it together, I was basically thinking of --driver-flavor=foo, > but we > > should get more buy in. > > --driver-flavor=foo sounds good to me. > > >> + const StringRef Value = Arg.drop_front(CCCOptName.size()); > >> + CCCMode M = llvm::StringSwitch<CCCMode>(Value) > >> + .Case("gcc", GCCMode) > >> + .Case("g++", GXXMode) > >> + .Case("cpp", CPPMode) > >> + .Default(InvalidMode); > > > > > > I bet you can get rid of InvalidMode by making the switch type unsigned > and > > returning ~0U in Default or something. > > Done. > > New patch attached. > > - Hans > > > On Mon, Jul 15, 2013 at 9:28 PM, Hans Wennborg <[email protected]> > wrote: > >> > >> On Mon, Jul 15, 2013 at 6:11 PM, David Blaikie <[email protected]> > wrote: > >> > (not claiming ownership of this CR, but a few drive-by comments) > >> > > >> > You could consider using a StringSwitch in Driver::ParseCCCMode. > >> > >> Done. It's a little annoying that I need an invalid value for the > >> default case, but on the other hand the StringSwitch is nice. > >> > >> > Is there any reason you need to remove support for "clangxx" (that's > >> > causing you to need to update those hexagon tests?)? > >> > >> I'm not, I'm switching those hexagon tests over to use clangxx instead > >> of -ccc-cxx which I'm removing. > >> > >> > Would you mind updating immediate-options.c to use FileCheck rather > >> > than multiple invocations with grep? > >> > >> Sure. I'd like to do that in a separate patch, though. > >> > >> Thanks for the comments! New patch attached. > >> > >> - Hans > >> > >> > >> > On Mon, Jul 15, 2013 at 4:17 PM, Hans Wennborg <[email protected]> > >> > wrote: > >> >> Hi all, > >> >> > >> >> This patch is a follow-up to the discussion in Reid's cl.exe > >> >> compatible driver proposal [1]. > >> >> > >> >> Clang currently looks at argv[0] to switch between running in > >> >> gcc/g++/cpp mode. This mode is represented in Driver by CCCIsCXX and > >> >> CCCIsCPP. Since there is no overlap between the three modes, I have > >> >> turned those two variables into an enum instead. The plan is to later > >> >> extend this enum with a "cl.exe mode". > >> >> > >> >> The patch also adds a new command line option, -ccc-mode, to set the > >> >> mode. This replaces the current -ccc-cxx option (and also makes it > >> >> easier to test the cpp mode). This option is special: because the > mode > >> >> can affect option parsing (the cl.exe mode will add new options), the > >> >> -ccc-mode needs to be parsed early. > >> >> > >> >> Please take a look! > >> >> > >> >> Thanks, > >> >> Hans > >> >> > >> >> [1]. > http://lists.cs.uiuc.edu/pipermail/cfe-dev/2013-June/030439.html >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
