<div dir="ltr">On Mon, Aug 12, 2013 at 11:20 AM, Thompson, John <span dir="ltr"><<a href="mailto:John_Thompson@playstation.sony.com" target="_blank">John_Thompson@playstation.sony.com</a>></span> wrote:<br><div class="gmail_extra">
<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-US" link="blue" vlink="purple">
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Son of a gun, I thought I looked and didn’t see the input file in the Args, but in checking again, there it is.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">However, this patch does make it easier, allowing me to avoid writing logic to determine which argument is the input file, since ClangTool had the input file
handy already. But I’ll defer to you. Do you think it’s helpful enough to put in, or would avoiding the change be better?</span></p></div></div></blockquote><div><br></div><div>I think putting the input file in as a special case isn't what we want - if we do that, what do we do if we need to extract other options when we modify them?</div>
<div><br></div><div>If we really need more tools to extract different parts of the options, I think the right way is to make parsing those options easier via a small library function.</div><div><br></div><div>Cheers,</div>
<div>/Manuel</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Thanks.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">-John<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Manuel Klimek [mailto:<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>]
<br>
<b>Sent:</b> Monday, August 12, 2013 9:16 AM</span></p><div><div class="h5"><br>
<b>To:</b> Thompson, John<br>
<b>Cc:</b> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<b>Subject:</b> Re: {PATCH] clang::tooling::ArgumentAdjuster - Add file name argument to Adjust callback<u></u><u></u></div></div><p></p><div><div class="h5">
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">On Mon, Aug 12, 2013 at 8:46 AM, Thompson, John <<a href="mailto:John_Thompson@playstation.sony.com" target="_blank">John_Thompson@playstation.sony.com</a>> wrote:<u></u><u></u></p>
<div>
<div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<p class="MsoNormal">Manuel,<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">Thanks for the feedback.<u></u><u></u></p>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">>first, I'd be eternally thankful if you submitted patches to Tooling via phabricator (<a href="http://llvm.org/docs/Phabricator.html" target="_blank">http://llvm.org/docs/Phabricator.html</a>)
:)<u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
</div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">I’ll give it a try.</span><u></u><u></u></p>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
<p class="MsoNormal">>Second, I'm not yet convinced this patch is the right direction. I'm trying to understand what you're doing:<u></u><u></u></p>
<p class="MsoNormal">>1. when running a tool over code, I'd assume you don't need to change the include file arguments<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Modularize reads a list of headers to individually compile and check. Some of these headers might
depend on other headers to be included first. This allows modularize to force the compiler to read first the headers that are depended on, so the main input header file will compile without errors.</span><u></u><u></u></p>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">>2. after run modularize, if I understand you correctly, the header search paths are now not correct any more - this looks to me like it needs build system support, rather than
trying to work around this with the include paths<u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
</div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">This doesn’t involve changing include paths. The -include file options added tell the compiler to
read those files first before compiling the main input file. There is no build system involved other than running modularize via a command line.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">The alternative is either to create a separate ClangTool object with the added -include options for
each header file to be checked, or change ClangTool to let me provide individual compile commands, probably needing a need CompilationDatabase class. Using ArgumentsAdjuster seems to be a more elegant approach. For example, here’s the new modularize code
showing the interaction with ClangTool:</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas;color:green">// We provide this derivation to add in "-include (file)" arguments for header</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas;color:green">// dependencies.</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas;color:blue">class</span><span style="font-size:9.5pt;font-family:Consolas"> AddDependenciesAdjuster :
<span style="color:blue">public</span> ArgumentsAdjuster {</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas;color:blue">public</span><span style="font-size:9.5pt;font-family:Consolas">:</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> AddDependenciesAdjuster(DependencyMap &Dependencies)</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> : Dependencies(Dependencies) {}</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas;color:blue">private</span><span style="font-size:9.5pt;font-family:Consolas">:</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> CommandLineArguments Adjust(llvm::StringRef InputFile,</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> <span style="color:blue">
const</span> CommandLineArguments &Args) {</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> DependentsVector &FileDependents = Dependencies[InputFile];</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> <span style="color:blue">int</span> Count = FileDependents.size();</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> <span style="color:blue">if</span> (Count == 0)</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> <span style="color:blue">
return</span> Args;</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> CommandLineArguments NewArgs(Args);</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> <span style="color:blue">for</span> (<span style="color:blue">int</span> Index = 0; Index < Count; ++Index) {</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> NewArgs.push_back(<span style="color:#a31515">"-include"</span>);</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> std::string File(</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> std::string(<span style="color:#a31515">"\""</span>) + FileDependents[Index] + std::string(<span style="color:#a31515">"\""</span>));</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> NewArgs.push_back(FileDependents[Index]);</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> }</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> <span style="color:blue">return</span> NewArgs;</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> }</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> DependencyMap &Dependencies;</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas">};</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">…</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> <span style="color:green">// Parse all of the headers, detecting duplicates.</span></span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> EntityMap Entities;</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> ClangTool Tool(*Compilations, Headers);</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> Tool.appendArgumentsAdjuster(<span style="color:blue">new</span> AddDependenciesAdjuster(Dependencies));</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> <span style="color:blue">int</span> HadErrors =</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> Tool.run(<span style="color:blue">new</span> ModularizeFrontendActionFactory(Entities, *PPTracker));</span><u></u><u></u></p>
<p class="MsoNormal" style="text-autospace:none">
<span style="font-size:9.5pt;font-family:Consolas"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Or would using a separate ClangTool instance for each header be actually more in line with how the
tooling system should work?</span><u></u><u></u></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Ok, after giving it some thought, I think your use cases makes sense - but can't you figure out the input file from the given args?<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">-John</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Manuel Klimek [mailto:<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>]
<br>
<b>Sent:</b> Monday, August 12, 2013 6:34 AM<br>
<b>To:</b> Thompson, John<br>
<b>Cc:</b> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<b>Subject:</b> Re: {PATCH] clang::tooling::ArgumentAdjuster - Add file name argument to Adjust callback</span><u></u><u></u></p>
<div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
<div>
<p class="MsoNormal">Hi John,<u></u><u></u></p>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">first, I'd be eternally thankful if you submitted patches to Tooling via phabricator (<a href="http://llvm.org/docs/Phabricator.html" target="_blank">http://llvm.org/docs/Phabricator.html</a>)
:)<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">Second, I'm not yet convinced this patch is the right direction. I'm trying to understand what you're doing:<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">1. when running a tool over code, I'd assume you don't need to change the include file arguments<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">2. after run modularize, if I understand you correctly, the header search paths are now not correct any more - this looks to me like it needs build system support, rather than trying
to work around this with the include paths<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">Cheers,<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">/Manuel<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
</div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"> <u></u><u></u></p>
<div>
<p class="MsoNormal">On Mon, Aug 12, 2013 at 6:16 AM, Thompson, John <<a href="mailto:John_Thompson@playstation.sony.com" target="_blank">John_Thompson@playstation.sony.com</a>> wrote:<u></u><u></u></p>
<div>
<div>
<p class="MsoNormal">This patch adds an input file name parameter to ArgumentAdjuster’s Adjust function in Tooling, in order to allow support of changes to a compile’s arguments based on the file name.<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">I’m in a testing phase for a new feature for modularize which allows you to add dependencies to the header files in the input header list. In using modularize for a real-world
set of headers, I found that some headers needed other headers to be included first, as opposed to including them themselves or relying on a master header to include the subset of headers in the right order. With this patch, I can create an ArgumentAdjuster
derivation that’s given a map with the dependencies keyed on the file name, and have the Adjust function look up the dependencies using the input file name and then add -include options to first include the depended-on headers. This seemed easier than other
changes or derivations that would need to be done.<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">Thanks.<u></u><u></u></p>
<p class="MsoNormal"><span style="color:#888888"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="color:#888888">-John</span><u></u><u></u></p>
<p class="MsoNormal"><span style="color:#888888"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="color:#888888"> </span><u></u><u></u></p>
</div>
</div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><u></u><u></u></p>
</div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div>
</div></div></div>
</div>
</blockquote></div><br></div></div>