[PATCH] Add support for dependency files

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.

Feedback appreciated. Thanks.

dep_file.patch (9.74 KB)

enhance_path.patch (1.48 KB)

Hello,

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.

Feedback appreciated. Thanks.

I've raised a bug to make sure that this patch isn't forgotten:

http://llvm.org/bugs/show_bug.cgi?id=2618

Hi Kovarththanan,

I'm really sorry for the delay. This is on my todo list, but I'm scrambling to keep up with the developer meeting looming. I'll take a look at it this weekend if not sooner. I apologize again for the delay, thanks for working on this!

-Chris

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

Hello Chris,

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

Chris Lattner 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.

dep_file_v2.patch (9.77 KB)