Ben Langmuir <[email protected]> writes: > LGTM. r211302 and r211303.
>> 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
