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

[Chris Lattner]

On Jul 26, 2008, at 3:16 AM, Kovarththanan Rajaratnam wrote:

> Hello,
>
> This patch is a first stab at implementing rudimentary support for  
> generation of dependency files (-MD and -MMD). This is implemented  
> by using the preprocessor callback functionality.

Hi Kovarththanan,

I'm sorry for the delay in getting back to this patch.  This is a  
great new feature and something that we need.

I committed your improvements to sys::Path here:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080811/065877.html

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.

+  if (!GenerateDependencyFile && !GenerateDependencyFileNoSysHeaders) {
+    if (!DependencyOutputFile.empty() || !DependencyTarget.empty() ||  
PhonyDependencyTarget)

Please stay in 80 columns (also a couple other places).


+  /// 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.



+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.


+  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.


Thanks again for working on this!

-Chris

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080812/9baf71e1/attachment.html>