[llvm-commits] [llvm] r117596 - /llvm/trunk/lib/System/Unix/Program.inc

[Mikhail Glushenkov]

Hi,

On Thu, Oct 28, 2010 at 05:28:12PM -0700, Dan Gohman wrote:
>
> >
> > Thanks, I've missed this. What about changing
> >
> >  if (progName.find('/') != std::string::npos && temp.canExecute())
> >    return temp;
> >
> > to
> >
> >  if (progName.find('/') != std::string::npos) {
> >    if (temp.canExecute())
> >      return temp;
> >    else
> >      return Path();
> >  }
> >
> > This would be consistent with the Win32 version.
>
> This would prevent clients from being able to distinguish
> between "not found" and "not executable".

Do they really need to? I think that those two cases are synonymous for the
purposes of FindProgramByName - "an executable called X was not found".

Besides, if clients want to distinguish between executable and non-executable
absolute paths they can use functions from the Path class. It is not possible to
do this *from client code* by just using FindProgramByName, because for absolute
paths it is the identity operation.

If FindProgramByName is given a path without slashes, it doesn't return a
non-executable path even if it is found in the PATH. Why should the behaviour
for absolute paths be different?

If we just care about the error message shown to the user, why not add an
optional output parameter ErrorMessage to FindProgramByName/FindExecutable?

> > Alternatively, we can change FindExecutable to take into account that canExecute
> > can be false for the path returned by FindProgramByName.
>
>
> My assumption is that it's perfectly acceptable for FindExecutable
> to return a non-executable path here; its caller will attempt to
> execute the path, get an error, and report the error to the user,

I think it's not. Currently, FindExecutable always succeeds (on unix-likes),
even if given a non-absolute path. FindExecutable("something") =
Path("$EXE_DIR").appendComponent("something"), which was not the original
intent.

> What problem are you trying to solve here?

I find the current behaviour confusing and I want it to be consistent across
platforms. If we decide to standardise on the Unix behaviour, then we should
also make the Win32 version of FindProgramByName return absolute paths
untouched, and mention this issue in the comments.

--
()  ascii ribbon campaign - against html e-mail
/\  www.asciiribbon.org   - against proprietary attachments