[PATCH] Teach CC1 to dump module dependencies to a directory

[Ben Langmuir]

> On Jun 19, 2014, at 10:31 AM, Justin Bogner <mail at justinbogner.com> wrote:
> 
> Ben Langmuir <blangmuir at apple.com> 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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140619/ade2ac46/attachment.html>