LGTM. > On Jun 19, 2014, at 11:53 AM, Justin Bogner <[email protected]> wrote: > > Ben Langmuir <[email protected]> writes: >> On Jun 19, 2014, at 10:31 AM, Justin Bogner <[email protected]> wrote: >> >> Ben Langmuir <[email protected]> writes: >> >> Hi Justin, >> >> Thanks for working on this! It’s looking pretty close. >> >> +/// Append the absolute path in Nested to the path given by Root. >> This will >> +/// remove directory traversal from the resulting nested path. >> +static void appendNestedPath(SmallVectorImpl<char> &Root, >> + SmallVectorImpl<char> &Nested) { >> + using namespace llvm::sys; >> + SmallVector<StringRef, 16> ComponentStack; >> + >> + StringRef Rel = path::relative_path(StringRef(Nested.begin(), >> Nested.size())); >> >> You seem to have manually inlined Nested.str() ;-) Maybe just make >> your Nested parameter a StringRef? >> >> Right, not sure what I was thinking there :). I'll pass in a StringRef >> instead. >> >> + // We need an absolute path to append to the root. >> + SmallString<256> AbsoluteSrc = Src; >> + fs::make_absolute(AbsoluteSrc); >> + // Build the destination path. >> + SmallString<256> Dest = Collector.getDest(); >> + size_t RootLen = Dest.size(); >> + appendNestedPath(Dest, AbsoluteSrc); >> >> Do we need to escape this somehow on Windows, since you might get C: >> in the middle of your path? >> >> And in general, will this work if Dest ends with a path separator? >> Then you would end up with // in the middle, which could potentially >> be eaten at some point (not sure). >> >> The call to path::relative_path in appendNestedPath takes care of both >> of these issues. It's strips off root_name (ie, C:) and root_directory >> (ie, /). >> >> It sure does, silly me. >> >> +bool ModuleDependencyListener::visitInputFile(StringRef Filename, >> bool IsSystem, >> + bool IsOverridden) >> { >> + if (!Collector.insertSeen(Filename)) >> + return true; >> + if (copyToRoot(Filename)) >> + Collector.setHasErrors(); >> + return true; >> +} >> >> This is clearer to me if you invert the first if, but you decide. >> if (Collector.insertSeen(Filename)) >> if (copyToRoot(Filename)) >> Collector.setHasErrors(); >> return true; >> >> Sure, I'm happy with either. >> >> +// RUN: find %t/vfs -type f | FileCheck %s -check-prefix=DUMP >> +// DUMP: AlsoDependsOnModule.framework/Headers/ >> AlsoDependsOnModule.h >> +// DUMP: >> Module.framework/Frameworks/SubFramework.framework/Headers/ >> SubFramework.h >> >> REQUIRES: shell, since you need ‘find’. This applies to both tests. >> Also you won’t get the path separators you expect on Windows. >> >> Hmm. Is there a good way to check if the files are created without find? >> Assuming there is, I'll change it use regex for the path separators, as >> I think the extra platform coverage here is worthwhile. >> >> I can’t think of a cleaner way to do this. >> >> This isn’t really the place to discuss llvm patches, but... >> >> + char *Buf = new char[BufSize]; >> >> If you don’t want to put 4 KB on the stack, how about std::vector with >> its data() method? >> >> Yeah, 4k seemed like a bit much for the stack (though, this is always a >> leaf call, so maybe it's fine). >> >> Is a vector really better here? Since I have to manually manage closing >> the files anyway, the new/delete doesn't feel out of place, and using a >> std::vector or a std::unique_ptr purely as an RAII container muddies up >> what this is doing a bit. >> >> I don’t feel strongly about it, so go with what you have. >> >> + for (;;) { >> + Bytes = read(ReadFD, Buf, BufSize); >> + if (Bytes <= 0) >> + break; >> + Bytes = write(WriteFD, Buf, Bytes); >> + if (Bytes <= 0) >> + break; >> + } >> >> This doesn’t seem sufficiently paranoid about the number of bytes >> actually written by ‘write’. >> >> Right. This should probably loop on the write as well. I'll update that. > > New patches attached. > > <clang-0001-Frontend-Add-a-CC1-flag-to-dump-module-dependencies.4.patch><llvm-0001-Support-Add-llvm-sys-fs-copy_file.3.patch>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
