Yes, it's a mess. This patch looks good to me.
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
