<div dir="ltr"><div>Sean,</div><div> </div><div>Thanks for the feedback.</div><div> </div><div>Regarding the usefulness of the feature in general, my opinion is that modules don't care if headers within the module have dependencies on other headers within the module.  Perhaps if the headers are in separate modules it could be problematic.  The user can still choose to fix the dependency problem by adding #includes in the headers.  Modularize will still help them find these dependencies by the error generated when the dependencies are not satisfied.  In this particular case, the set of headers I'm checking are a platform's standard headers, so I'm trying to minimize the changes to them, and this change lets me test headers despite the dependencies, but I explicitly have to specify the needed headers.</div>
<div> </div><div>However, I'm speaking very subjectively, since I have no idea what the standard for modules is.  Does anyone have more objective information with respect to modules?  Doug?</div><div> </div><div>Thanks.</div>
<div> </div><div>-John</div><div> </div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Aug 23, 2013 at 11:31 AM, Sean Silva <span dir="ltr"><<a href="mailto:silvas@purdue.edu" target="_blank">silvas@purdue.edu</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
  Does this patch make sense from a design standpoint? Aren't these kinds of implicit dependencies the things that modularize is trying to get rid of in the first place? Why would we expend effort to enshrine them, when we can just detect them and emit an error?<br>

<br>
  I've also made a few comments about the code, but I think the above design issue is more important.<br>
<br>
<br>
================<br>
Comment at: modularize/Modularize.cpp:281<br>
@@ +280,3 @@<br>
+      std::string File(<br>
+        std::string("\"") + FileDependents[Index] + std::string("\""));<br>
+      NewArgs.push_back(FileDependents[Index]);<br>
----------------<br>
Is the quoting here actually necessary? I don't think that there is a shell-escaping step after this.<br>
<br>
================<br>
Comment at: modularize/Modularize.cpp:292-294<br>
@@ +291,5 @@<br>
+    SmallVector<const char*, 256> Argv;<br>
+    for (CommandLineArguments::const_iterator I = CLArgs.begin(),<br>
+          E = CLArgs.end();<br>
+        I != E; ++I)<br>
+      Argv.push_back(I->c_str());<br>
----------------<br>
clang-format<br>
<br>
================<br>
Comment at: modularize/Modularize.cpp:287-302<br>
@@ +286,18 @@<br>
+  // Helper function for finding the input file in an arguments list.<br>
+  llvm::StringRef findInputFile(const CommandLineArguments &CLArgs) {<br>
+    OwningPtr<OptTable> Opts(createDriverOptTable());<br>
+    const unsigned IncludedFlagsBitmask = options::CC1Option;<br>
+    unsigned MissingArgIndex, MissingArgCount;<br>
+    SmallVector<const char*, 256> Argv;<br>
+    for (CommandLineArguments::const_iterator I = CLArgs.begin(),<br>
+          E = CLArgs.end();<br>
+        I != E; ++I)<br>
+      Argv.push_back(I->c_str());<br>
+    OwningPtr<InputArgList> Args(<br>
+      Opts->ParseArgs(Argv.data(), Argv.data() + Argv.size(),<br>
+                      MissingArgIndex, MissingArgCount,<br>
+                      IncludedFlagsBitmask));<br>
+    std::vector<std::string> Inputs = Args->getAllArgValues(OPT_INPUT);<br>
+    return Inputs.back();<br>
+  }<br>
+  DependencyMap &Dependencies;<br>
----------------<br>
Does this function actually use any of the class members? It seems like it should be a free function.<br>
<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D1474" target="_blank">http://llvm-reviews.chandlerc.com/D1474</a><br>
</blockquote></div><br><br clear="all"><br>-- <br>John Thompson<br><a href="mailto:John.Thompson.JTSoftware@gmail.com">John.Thompson.JTSoftware@gmail.com</a><br>
</div>