On Fri, Jan 9, 2015 at 12:16 AM, Yaron Keren <[email protected]> wrote: > Yes, it's a mess. > This patch looks good to me.
Thanks! I added a test and committed in r225530. > 2015-01-09 1:24 GMT+02:00 Hans Wennborg <[email protected]>: >> >> 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 <[email protected]> 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 <[email protected]>: >> >> >> >> 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 <[email protected]>: >> >>> >> >>> On Sun, Jan 4, 2015 at 5:48 AM, Yaron Keren <[email protected]> >> >>> 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 >> >> >> >> >> > > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
