Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline
Changeset View
Standalone View
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
| Show First 20 Lines • Show All 60 Lines • ▼ Show 20 Lines | static cl::opt<bool> | ||||||||
| NonDeterministicOption("U", cl::desc("Use actual timestamps and UIDs/GIDs"), | NonDeterministicOption("U", cl::desc("Use actual timestamps and UIDs/GIDs"), | ||||||||
| cl::init(false), cl::cat(LibtoolCategory)); | cl::init(false), cl::cat(LibtoolCategory)); | ||||||||
| static cl::opt<std::string> | static cl::opt<std::string> | ||||||||
| FileList("filelist", | FileList("filelist", | ||||||||
| cl::desc("Pass in file containing a list of filenames"), | cl::desc("Pass in file containing a list of filenames"), | ||||||||
| cl::value_desc("listfile[,dirname]"), cl::cat(LibtoolCategory)); | cl::value_desc("listfile[,dirname]"), cl::cat(LibtoolCategory)); | ||||||||
| static cl::list<std::string> Libraries( | |||||||||
| "l", | |||||||||
| cl::desc( | |||||||||
| "l<x> searches for the library libx.a in the library search path. If" | |||||||||
| " the string 'x' ends with '.o', then the library 'x' is searched for" | |||||||||
| " without prepending 'lib' or appending '.a'"), | |||||||||
jhenderson: I just want to make sure the behaviour here is definitely what's documented and that it doesn't… | |||||||||
Yup, the behavior here is definitely what's documented. My first test case is testing this only where I pass in -larchive-object.o. Also, I have now changed that test by passing in -l%basename_t.tmp-input1.o instead of archive-object as per @smeenai's recommendation above. sameerarora101: Yup, the behavior here is definitely what's documented. My first test case is testing this only… | |||||||||
I think @jhenderson meant that we should also add a case where you have e.g. a file called libfoo.o.a, you pass -lfoo.o, and you make sure that you get an error about a missing file (i.e. confirming that when you have a -l argument ending in .o, you only search for the name as-is). smeenai: I think @jhenderson meant that we should also add a case where you have e.g. a file called… | |||||||||
Oh, i see now. I'll add a test. Thanks for clarifying! sameerarora101: Oh, i see now. I'll add a test. Thanks for clarifying! | |||||||||
| cl::ZeroOrMore, cl::Prefix, cl::cat(LibtoolCategory)); | |||||||||
| static cl::list<std::string> LibrarySearchDirs( | |||||||||
How about LibrarySearchDirs? smeenai: How about `LibrarySearchDirs`? | |||||||||
| "L", | |||||||||
| cl::desc( | |||||||||
| "L<dir> adds <dir> to the list of directories in which to search for" | |||||||||
| " libraries"), | |||||||||
| cl::ZeroOrMore, cl::Prefix, cl::cat(LibtoolCategory)); | |||||||||
| static const std::array<std::string, 3> StandardSearchDirs{ | |||||||||
Nit: I'd prefer a std::array. I think you can make it constexpr too? I'd also prefer StandardSearchDirs. smeenai: Nit: I'd prefer a `std::array`. I think you can make it `constexpr` too?
I'd also prefer… | |||||||||
Seems like I cannot have constexpr with std::array<std::string, 3>. It says, "constexpr variable cannot have non-literal type 'const std::array<std::string, 3>' (aka 'const array<basic_string<char>, 3>')". sameerarora101: Seems like I cannot have `constexpr` with `std::array<std::string, 3>`. It says, "constexpr… | |||||||||
Ah, oh well. smeenai: Ah, oh well. | |||||||||
| "/lib", | |||||||||
| "/usr/lib", | |||||||||
| "/usr/local/lib", | |||||||||
I don't think you need the trailing slashes. smeenai: I don't think you need the trailing slashes. | |||||||||
| }; | |||||||||
I defined StandarDirs based on cctools' libtool: /* the standard directories to search for -lx names */
char *standard_dirs[] = {
"/lib/",
"/usr/lib/",
"/usr/local/lib/",
NULL
};sameerarora101: I defined `StandarDirs` based on cctools' libtool:
```
/* the standard directories to search… | |||||||||
| struct Config { | struct Config { | ||||||||
| bool Deterministic = true; // Updated by 'D' and 'U' modifiers. | bool Deterministic = true; // Updated by 'D' and 'U' modifiers. | ||||||||
| uint32_t ArchCPUType; | uint32_t ArchCPUType; | ||||||||
| uint32_t ArchCPUSubtype; | uint32_t ArchCPUSubtype; | ||||||||
| }; | }; | ||||||||
| static Expected<std::string> searchForFile(const Twine &FileName) { | |||||||||
Could this be moved into the loop body, to avoid the Path.clear() line? jhenderson: Could this be moved into the loop body, to avoid the `Path.clear()` line? | |||||||||
| auto FindLib = | |||||||||
| [FileName](ArrayRef<std::string> SearchDirs) -> Optional<std::string> { | |||||||||
| for (StringRef Dir : SearchDirs) { | |||||||||
| SmallString<128> Path; | |||||||||
Any reason for creating this variable? SmallString can be implicitly converted to StringRef instead, and it can also be converted to std::string explicitly (e.g. via std::string(Path)). smeenai: Any reason for creating this variable? SmallString can be implicitly converted to StringRef… | |||||||||
Ah forgot about the explicit conversion, thanks! sameerarora101: Ah forgot about the explicit conversion, thanks! | |||||||||
| sys::path::append(Path, Dir, FileName); | |||||||||
Nit: you don't need the braces smeenai: Nit: you don't need the braces | |||||||||
| if (sys::fs::exists(Path)) | |||||||||
| return std::string(Path); | |||||||||
| } | |||||||||
I'd prefer an Optional<std::string> instead of having the empty string represent the absence. smeenai: I'd prefer an `Optional<std::string>` instead of having the empty string represent the absence. | |||||||||
| return None; | |||||||||
| }; | |||||||||
| Optional<std::string> Found = FindLib(LibrarySearchDirs); | |||||||||
The duplication between the two loops is unfortunate, but I don't think the body of the loops is large enough to justify moving it out into a function (especially if you get rid of the PathString variable). What we really want is a C++ equivalent to Python's itertools.chain, and I think we might get that with ranges in C++20. smeenai: The duplication between the two loops is unfortunate, but I don't think the body of the loops… | |||||||||
Perhaps a lambda could work okay here: auto FindLib = [](ArrayRef<StringRef> SearchDirs) {
for (StringRef Dir : SearchDirs) {
SmallString<128> Path;
sys::path::append(Path, Dir, FileName);
if (sys::fs::exists(Path))
return std::string(Path);
}
return "";
};
std::string Found = FindLib(LibrarySearchDirs);
if (Found.empty())
Found = FindLib(StandardSearchDirs);
if (!Found.empty())
return Found;
// report errorjhenderson: Perhaps a lambda could work okay here:
```
auto FindLib = [](ArrayRef<StringRef> SearchDirs) {… | |||||||||
I like the lambda too, updated now. Thanks! sameerarora101: I like the lambda too, updated now. Thanks! | |||||||||
| if (!Found.hasValue()) | |||||||||
| Found = FindLib(StandardSearchDirs); | |||||||||
| if (Found.hasValue()) | |||||||||
| return Found.getValue(); | |||||||||
| return createStringError(std::errc::invalid_argument, | |||||||||
jhenderson: | |||||||||
it is not trying to locate '%s' but might add lib and .a to the string and try locating lib%s.a, right? That's what I meant up there. I can change it to what you're suggesting if it doesn't make sense? sameerarora101: it is not trying to locate `'%s'` but might add `lib` and `.a` to the string and try locating… | |||||||||
FileName will have already had the lib and .a additions at this point though, right? smeenai: `FileName` will have already had the `lib` and `.a` additions at this point though, right? | |||||||||
ah yes, you are right. Replaced as suggested. sameerarora101: ah yes, you are right. Replaced as suggested. | |||||||||
| "cannot locate file '%s'", FileName.str().c_str()); | |||||||||
| } | |||||||||
| static Error processCommandLineLibraries() { | |||||||||
| for (StringRef BaseName : Libraries) { | |||||||||
Can't you just do for(StringRef BaseName : Libraries)? If not, at least do s -> S jhenderson: Can't you just do `for(StringRef BaseName : Libraries)`?
If not, at least do s -> S | |||||||||
It's not particularly idiomatic to put const with StirngRef. jhenderson: It's not particularly idiomatic to put `const` with `StirngRef`. | |||||||||
| Expected<std::string> FullPath = searchForFile( | |||||||||
| BaseName.endswith(".o") ? BaseName.str() : "lib" + BaseName + ".a"); | |||||||||
| if (!FullPath) | |||||||||
const & smeenai: `const &` | |||||||||
| return FullPath.takeError(); | |||||||||
One of the following should work, I think. StringRef BaseName = s; StringRef BaseName(s); smeenai: One of the following should work, I think.
```
StringRef BaseName = s;
StringRef BaseName(s)… | |||||||||
Both work 😄 sameerarora101: Both work 😄 | |||||||||
| InputFiles.push_back(FullPath.get()); | |||||||||
| } | |||||||||
| return Error::success(); | |||||||||
I would construct a Twine and have searchForFile take const Twine & instead. smeenai: I would construct a `Twine` and have `searchForFile` take `const Twine &` instead. | |||||||||
Seems like I can't construct a Twine before the if condition and modify it appropriately in the corresponding branches (which is needed so as to have the Twine variable in scope after the if condition). Furthermore, I also cannot directly call searchForFile(<Twine>); within each branch and assign the result to Expected<std::string> FullPath, as I cannot declare Expected<std::string> FullPath; before the if branch. Sorry if I didn't get what you were trying to explain here ? sameerarora101: Seems like I can't construct a `Twine` before the `if` condition and modify it appropriately in… | |||||||||
Not Done ReplyInline ActionsHmm, good points. Lemme think if there's anything else we can do here. smeenai: Hmm, good points. Lemme think if there's anything else we can do here. | |||||||||
How about something like: Expected<std::string> FullPath = searchForFile(BaseName.endswith(".o") ?
BaseName.str() :
"lib" + BaseName + ".a");jhenderson: How about something like:
```
Expected<std::string> FullPath = searchForFile(BaseName.endswith… | |||||||||
works, thanks. sameerarora101: works, thanks. | |||||||||
Ah, I keep forgetting about ternary expressions. smeenai: Ah, I keep forgetting about ternary expressions. | |||||||||
I avoid them in most cases, but this seems like a sensible usage. jhenderson: I avoid them in most cases, but this seems like a sensible usage. | |||||||||
| } | |||||||||
| static Error processFileList() { | static Error processFileList() { | ||||||||
| StringRef FileName, DirName; | StringRef FileName, DirName; | ||||||||
| std::tie(FileName, DirName) = StringRef(FileList).rsplit(","); | std::tie(FileName, DirName) = StringRef(FileList).rsplit(","); | ||||||||
| ErrorOr<std::unique_ptr<MemoryBuffer>> FileOrErr = | ErrorOr<std::unique_ptr<MemoryBuffer>> FileOrErr = | ||||||||
| MemoryBuffer::getFileOrSTDIN(FileName, /*FileSize=*/-1, | MemoryBuffer::getFileOrSTDIN(FileName, /*FileSize=*/-1, | ||||||||
| /*RequiresNullTerminator=*/false); | /*RequiresNullTerminator=*/false); | ||||||||
| if (std::error_code EC = FileOrErr.getError()) | if (std::error_code EC = FileOrErr.getError()) | ||||||||
| ▲ Show 20 Lines • Show All 305 Lines • ▼ Show 20 Lines | static Expected<Config> parseCommandLine(int Argc, char **Argv) { | ||||||||
| cl::ParseCommandLineOptions(Argc, Argv, "llvm-libtool-darwin\n"); | cl::ParseCommandLineOptions(Argc, Argv, "llvm-libtool-darwin\n"); | ||||||||
| if (DeterministicOption && NonDeterministicOption) | if (DeterministicOption && NonDeterministicOption) | ||||||||
| return createStringError(std::errc::invalid_argument, | return createStringError(std::errc::invalid_argument, | ||||||||
| "cannot specify both -D and -U flags"); | "cannot specify both -D and -U flags"); | ||||||||
| else if (NonDeterministicOption) | else if (NonDeterministicOption) | ||||||||
| C.Deterministic = false; | C.Deterministic = false; | ||||||||
| if (!Libraries.empty()) | |||||||||
| if (Error E = processCommandLineLibraries()) | |||||||||
| return std::move(E); | |||||||||
| if (!FileList.empty()) | if (!FileList.empty()) | ||||||||
| if (Error E = processFileList()) | if (Error E = processFileList()) | ||||||||
| return std::move(E); | return std::move(E); | ||||||||
| if (InputFiles.empty()) | if (InputFiles.empty()) | ||||||||
| return createStringError(std::errc::invalid_argument, | return createStringError(std::errc::invalid_argument, | ||||||||
| "no input files specified"); | "no input files specified"); | ||||||||
| Show All 31 Lines | |||||||||
I just want to make sure the behaviour here is definitely what's documented and that it doesn't search for libfoo.o.a for -lfoo.o? We probably need a test case for that.