<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Jul 26, 2008, at 3:16 AM, Kovarththanan Rajaratnam wrote:</div><br><blockquote type="cite"><div>Hello,<br><br>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.<br></div></blockquote></div><br><div>Hi Kovarththanan,</div><div><br></div><div>I'm sorry for the delay in getting back to this patch. This is a great new feature and something that we need.</div><div><br></div><div>I committed your improvements to sys::Path here:</div><div><a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080811/065877.html">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080811/065877.html</a></div><div><br></div><div>I have a couple of comments on the rest of the patch:</div><div><br></div><div><div>+/// CreateDependencyFileGen - Create dependency file generator.</div> <div>+/// This is only done if either -MD or -MMD has been specified.</div> <div>+bool CreateDependencyFileGen(Preprocessor *PP, </div> <div>+<span class="Apple-tab-span" style="white-space:pre"> </span> std::string &OutputFile, </div> <div>+<span class="Apple-tab-span" style="white-space:pre"> </span> const std::string &InputFile, </div> <div>+<span class="Apple-tab-span" style="white-space:pre"> </span> bool PreprocessInputFile,</div> <br></div><div>Please use spaces instead of tabs.</div><div><br></div><div><div>+ if (!GenerateDependencyFile && !GenerateDependencyFileNoSysHeaders) {</div> <div>+ if (!DependencyOutputFile.empty() || !DependencyTarget.empty() || PhonyDependencyTarget)</div> <br></div><div>Please stay in 80 columns (also a couple other places).</div><div><br></div><div><br></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">+ /// FIXME: PP can only handle one callback</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">+ if (PreprocessInputFile)</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">+ return false;</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "><br></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">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.</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "><br></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "><br></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "><br></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">+const std::string DependencyFileExt("d");</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">+const std::string ObjectFileExt("o");</div><div><br></div></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">This causes static constructors to be run. Is there any reason not to include these inline? If you want to keep them, please use:</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "><br></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">static const char DependencyFileExt[] = "d";</div><div><br></div></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">which is more efficient.</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "><br></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "><br></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">+ llvm::SetVector<const char*> Files;</div><div><br></div><div>This won't do what you want, it will unique based on the address of temporary strings. I'd suggest using a StringSet instead.</div><div><br></div><div><br></div><div>Thanks again for working on this!</div><div><br></div><div>-Chris</div></div><div><font class="Apple-style-span" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;"><br></span></font></div></div></body></html>