r225134 - Fix default image name to 'a.exe' on Windows, instead 'a.out'.

[Yaron Keren]

Yes, it's a mess.
This patch looks good to me.




2015-01-09 1:24 GMT+02:00 Hans Wennborg <hans at chromium.org>:

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