<div dir="rtl"><div dir="ltr">Yes, it's a mess. </div><div dir="ltr">This patch looks good to me.<br></div><div dir="ltr"><br></div><div dir="ltr"><br></div><div dir="ltr"><br></div></div><div class="gmail_extra"><br><div class="gmail_quote"><div dir="ltr">2015-01-09 1:24 GMT+02:00 Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>></span>:</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hmm, the whole thing seems a bit of a mess :/<br>
<br>
However, I don't think we need to base DefaultImageName on the full<br>
logic of computeTargetTriple(), which is mostly fine-tuning the triple<br>
based on various flags. I think it's enough to make DefaultImageName<br>
match DefaultTargetTriple, like in the attached patch. What do you<br>
think?<br>
<div class="HOEnZb"><div class="h5"><br>
On Thu, Jan 8, 2015 at 2:38 PM, Yaron Keren <<a href="mailto:yaron.keren@gmail.com">yaron.keren@gmail.com</a>> wrote:<br>
> Rethinking, my suggestion is not good. getToolChain is called several times<br>
> so returning the target triple and setting DefaultImageName baed on it<br>
> duplicates the same operation several times.<br>
><br>
> Moving DefaultImageName to llvm::Triple or adding the helper function will<br>
> not help, as the the root of the problem is that the target triple is not<br>
> known when it's needed: It is computed in the const getToolChain but not<br>
> stored.<br>
> Storing in Driver::TargetTriple would make getToolChain non-const or or<br>
> Driver::TargetTriple mutuable just the same as DefaultImageName, which isn't<br>
> cleaner.<br>
><br>
> Also, look at the FIXME at line 339 which seems to workaround the same const<br>
> issue. Maybe we can solve both.<br>
><br>
> What do you think?<br>
><br>
><br>
><br>
> 2015-01-08 23:59 GMT+02:00 Yaron Keren <<a href="mailto:yaron.keren@gmail.com">yaron.keren@gmail.com</a>>:<br>
>><br>
>> The Driver needs DefaultImageName which is based on the target triple but<br>
>> the only location the target triple is computed using computeTargetTriple()<br>
>> is in getToolChain() and the target triple is not kept otherwise.<br>
>><br>
>> We could have getToolChain return the computed target triple to its caller<br>
>> BuildCompilation (non const mmeber function) and have BuildCompilation set<br>
>> DefaultImageName based on the OS. Does this make sense to you?<br>
>><br>
>> I'll fix the test.<br>
>><br>
>><br>
>><br>
>><br>
>><br>
>><br>
>> 2015-01-08 23:16 GMT+02:00 Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>>:<br>
>>><br>
>>> On Sun, Jan 4, 2015 at 5:48 AM, Yaron Keren <<a href="mailto:yaron.keren@gmail.com">yaron.keren@gmail.com</a>><br>
>>> wrote:<br>
>>> > Author: yrnkrn<br>
>>> > Date: Sun Jan 4 07:48:30 2015<br>
>>> > New Revision: 225134<br>
>>> ><br>
>>> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=225134&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=225134&view=rev</a><br>
>>> > Log:<br>
>>> > Fix default image name to 'a.exe' on Windows, instead 'a.out'.<br>
>>> > This applies to mingw as clang-cl already has its own logic for the<br>
>>> > filename.<br>
>>><br>
>>> [..]<br>
>>><br>
>>> > Modified: cfe/trunk/lib/Driver/Driver.cpp<br>
>>> > URL:<br>
>>> > <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Driver.cpp?rev=225134&r1=225133&r2=225134&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Driver.cpp?rev=225134&r1=225133&r2=225134&view=diff</a><br>
>>> ><br>
>>> > ==============================================================================<br>
>>> > --- cfe/trunk/lib/Driver/Driver.cpp (original)<br>
>>> > +++ cfe/trunk/lib/Driver/Driver.cpp Sun Jan 4 07:48:30 2015<br>
>>> > @@ -1996,6 +1996,8 @@ const ToolChain &Driver::getToolChain(co<br>
>>> > StringRef DarwinArchName) const<br>
>>> > {<br>
>>> > llvm::Triple Target = computeTargetTriple(DefaultTargetTriple, Args,<br>
>>> > DarwinArchName);<br>
>>> > + if (Target.isOSWindows())<br>
>>> > + DefaultImageName = "a.exe";<br>
>>><br>
>>> This doesn't seem right. Why should getToolChain() be changing the<br>
>>> Driver's DefaultImageName? That this requires DefaultImageName to be<br>
>>> mutable makes it feel even more iffy.<br>
>>><br>
>>> Maybe DefaultImageName should be a property of the Target? Or perhaps<br>
>>> there should be a "const char *Driver::DefaultImageName(llvm::Triple)"<br>
>>> method?<br>
>>><br>
>>><br>
>>> > Modified: cfe/trunk/test/Driver/lto.c<br>
>>><br>
>>> It would be nice to test for the default image name explicitly by<br>
>>> setting -target and looking at the cc1 invocation.<br>
>>><br>
>>> - Hans<br>
>><br>
>><br>
><br>
</div></div></blockquote></div><br></div>