2009/12/5 Rafael Espíndola <[email protected]>: >> I'm not really happy about this patch (adding argument parsing logic >> in tools/driver in particular). > > I am not happy with where it is parsed too. I just couldn't find a > better place without a lot additional refactoring.
I view this as a symptom of it being a "bad idea". I really dislike providing command line options for this kind of thing -- its better to find a model where the compiler "just works". >> Is it also really true that this is >> all this option does for gcc? > > Gcc does path canonization in a central location, but in practical > term it looks like the only difference is where gcc looks for cc1. > Note that gcc's cc1 never tries to find its own canonical path. In > clang I will probably have to pass -no-canonical-prefixes to clang-cc > (or clang -cc1) too. I intend to have clang pass the "resources path" to clang-cc/clang -cc1 fairly soon, we shouldn't need this option at the clang-cc level. >> I assume this is fixing something in a google symlink madness >> environment, but I think there is a better solution. > > It makes clang work on what Simon calls a "symlink farm". Consider > > farm/bin/clang -> /foo/clang > farm/bin/clang-cc -> /bar/clang-cc Yes, I understand the use case. >> The main thing we >> are using this for is to find our "resources" (lib files and includes) >> -- what if we searched the following locations in order: >> a. relative to argv[0] >> b. relative to argv[0], following top-level links (NOT realpath) >> c. relative to GetMainExecutable() >> >> I suspect this would solve your problem, and would also find the >> library directory much faster (than dladdr + realpath and friends) in >> the common case. > > I would not match gcc's behavior. Is that actually a problem? I'm not convinced these parts of gcc are well designed. > I am not sure if it might really be > a problem, but consider case where a resource can be found with "b" > but not with "a". I would like clang and clang-cc to produce an error. Is this important (the error)? Having a single consistent model is much friendlier to users (in the long run) than having multiple models and requiring an option. I think it is very unlikely that this lookup will (a) succeed and (b) find the wrong compiler; do you have a use case where that would happen? > Also, it would be slower when a resource cannot be found. This is not a use case that matters, though. >> I have a few other nits re the patch, inline, for posterity. >> >>> Modified: >>> cfe/trunk/include/clang/Driver/Options.td >>> cfe/trunk/lib/Driver/Tools.cpp >>> cfe/trunk/tools/driver/driver.cpp >>> >>> Modified: cfe/trunk/include/clang/Driver/Options.td >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=90577&r1=90576&r2=90577&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/include/clang/Driver/Options.td (original) >>> +++ cfe/trunk/include/clang/Driver/Options.td Fri Dec 4 13:31:58 2009 >>> @@ -375,6 +375,8 @@ >>> def mwarn_nonportable_cfstrings : Flag<"-mwarn-nonportable-cfstrings">, >>> Group<m_Group>; >>> def m_Separate : Separate<"-m">, Group<m_Group>; >>> def m_Joined : Joined<"-m">, Group<m_Group>; >>> +def no_canonical_prefixes : Flag<"-no-canonical-prefixes">, >>> Flags<[DriverOption]>, >>> + HelpText<"Do not resolve symbolic links, turn relative paths into >>> absolute ones, or do anything else to identify the executable">; >> >> This should probably be under help hidden, or perhaps just in the man >> page, its certainly not something to make the "short" help list. > > How about "Use relative instead of canonical paths"? My specific comment was just that the option shouldn't be in --help. There is a new flag for making it hidden by default. >>> def no_cpp_precomp : Flag<"-no-cpp-precomp">; >>> def no_integrated_cpp : Flag<"-no-integrated-cpp">, Flags<[DriverOption]>; >>> def no__dead__strip__inits__and__terms : >>> Flag<"-no_dead_strip_inits_and_terms">; >>> >>> Modified: cfe/trunk/lib/Driver/Tools.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=90577&r1=90576&r2=90577&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/lib/Driver/Tools.cpp (original) >>> +++ cfe/trunk/lib/Driver/Tools.cpp Fri Dec 4 13:31:58 2009 >>> @@ -1104,6 +1104,9 @@ >>> // care to warn the user about. >>> Args.ClaimAllArgs(options::OPT_clang_ignored_f_Group); >>> Args.ClaimAllArgs(options::OPT_clang_ignored_m_Group); >>> + >>> + // -no-canonical-prefixes is used very early in main. >>> + Args.ClaimAllArgs(options::OPT_no_canonical_prefixes); >> >> This isn't the right place to claim, the option was handled in the >> main() so it should be claimed in the driver. But the only reason this >> is needed at all is because the argument parsing was done by hand. > > I can try to change where we claim it, but I think I have to parse > this one by hand (as -cc1 is) since it has to be parsed before the > option parsing infrastructure is up and running. > > Cheers, > Rafael > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
