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