[cfe-dev] [PATCH] Add support for dependency files

[Kovarththanan Rajaratnam]

Hello Chris,

I somehow missed your review comments. Sorry for the delayed response.

Chris Lattner wrote:
> 
> On Jul 26, 2008, at 3:16 AM, Kovarththanan Rajaratnam wrote:
> 
> I have a couple of comments on the rest of the patch:
> 
> +/// CreateDependencyFileGen - Create dependency file generator.
> +/// This is only done if either -MD or -MMD has been specified.
> +bool CreateDependencyFileGen(Preprocessor *PP, 
> + std::string &OutputFile, 
> + const std::string &InputFile, 
> + bool PreprocessInputFile,
> 
> Please use spaces instead of tabs.

Fixed.

> +  if (!GenerateDependencyFile && !GenerateDependencyFileNoSysHeaders) {
> +    if (!DependencyOutputFile.empty() || !DependencyTarget.empty() || 
> PhonyDependencyTarget)
> 
> Please stay in 80 columns (also a couple other places).

Fixed.

> +  /// FIXME: PP can only handle one callback
> +  if (PreprocessInputFile)
> +    return false;
> 
> The idea behind the callbacks is that they could be chained together. 
>  You could have the dependency file codegen take an input set of 
> callbacks and invoke forward each method as it is called.  In addition 
> to chaining them, please consider checking 'PreprocessInputFile' in the 
> caller of CreateDependencyFileGen.  This would make the code easier to read.

I've moved the check to the caller as suggested. There's still the one 
callback limitation. I wondering whether it would make more sense to 
extend the preprocessor to handle multiple listeners instead. This seems 
to put less burden on the users of clang.

> 
> +const std::string DependencyFileExt("d");
> +const std::string ObjectFileExt("o");
> 
> This causes static constructors to be run.  Is there any reason not to 
> include these inline?  If you want to keep them, please use:
> 
> static const char DependencyFileExt[] = "d";
> 
> which is more efficient.

Fixed (by using the const char solution). I haven't included them inline 
  because it might make sense to be able to configure these?

> 
> +  llvm::SetVector<const char*> Files;
> 
> This won't do what you want, it will unique based on the address of 
> temporary strings.  I'd suggest using a StringSet instead.

Fixed.

Thanks for you comments.

-- 
Best Regards
Kovarththanan Rajaratnam
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dep_file_v2.patch
Type: text/x-patch
Size: 10000 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080823/ed7e64cd/attachment-0001.bin>