<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On May 5, 2013, at 22:19 , Joshua Cranmer <<a href="mailto:pidgeot18@gmail.com">pidgeot18@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<div bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 5/3/2013 6:18 PM, Jordan Rose wrote:<br>
</div>
<blockquote cite="mid:EF146988-CF53-4A2C-8194-D17CC7DA1E2F@apple.com" type="cite">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<div>
<blockquote type="cite" style="font-family: Menlo-Regular;
font-size: 11px;">
<blockquote type="cite">We really ought to have a preliminary
discussion on cfe-dev to map out the directions we want to
take the plugin API, and what *compelling*, *enabling*
changes we can make to it.<br>
</blockquote>
</blockquote>
<br>
</div>
<div>While I mostly like what you've come up with, Sean has a
point that the community hasn't really picked it up. I have your
e-mail from a year ago still flagged ("[cfe-dev] Thinking about
clang plugins"). No one responded to it then, but I think it was
a good summary of the problems you were/are trying to solve. I
would say to bring that up again...except most likely nothing
will happen until after the 3.3 release (and for us Apple folks,
after WWDC).</div>
</blockquote>
<br>
I had originally wanted to get this patch into the 3.1 release. That
nothing can come before 3.3 is... disheartening, to say the least.<br></div></blockquote><div><br></div><div>I know. I think the general lack of interest from the community says a few things about this: the general response to your new architecture has been "mildly positive" or thereabouts, but nobody's been dying for this and so nobody's shaping it in any significant way. Adding new UI that needs to be future-proof should always be taken seriously</div><div><br></div><div>But on the other hand, you have had this up for a long time, and I can see how this is a stepping-stone to more significant improvements (I like where the custom C++11 attributes idea is going). So I won't block this, and I appreciate you working on it.</div><div><br></div><br><blockquote type="cite"><div bgcolor="#FFFFFF" text="#000000">
<blockquote cite="mid:EF146988-CF53-4A2C-8194-D17CC7DA1E2F@apple.com" type="cite">
<blockquote type="cite">
<div>
<div>+Clang Plugins run a set of callbacks over code. These
callbacks are set up by</div>
<div>+the plugin when the compiler calls a function exported
by the plugin. The main</div>
<div>+function exported by the plugin is
clang_plugin_begin_tu, which is called before</div>
<div>+any file is compiled.</div>
</div>
</blockquote>
<div><br>
</div>
<div>s/any/each/, I think. Is there a separate callback that is
guaranteed to only be called once, or at least once per
CompilerInstance?</div>
</blockquote>
<br>
If you call clang a.cpp b.cpp, the last time I checked, this spawns
two clang -cc1 processes. Since the plugin is loaded in the clang
-cc1 process, it gets loaded twice, so call-per-invocation is
effectively equivalent to call-per-translation unit. My original
implementation had a call-per-invocation callback, but I removed it
after discovering this fact since its effect were misleading.<br></div></blockquote><div><br></div><div>This is true of the Clang driver, but not necessarily true of a (hypothetical) clang compilation or indexing server. Then again, leaving out the callback for now doesn't preclude adding it later.</div><div><br></div><br><blockquote type="cite"><div bgcolor="#FFFFFF" text="#000000"><blockquote cite="mid:EF146988-CF53-4A2C-8194-D17CC7DA1E2F@apple.com" type="cite"><blockquote type="cite"><div><div>+/// A struct containing sufficient information to
express ABI compatibility for</div>
<div>+/// plugins. It is sufficient for plugins to use the</div>
<div>+/// DECLARE_PLUGIN_COMPATIBILITY macro to find
compatibility.</div>
<div>+struct Version {</div>
<div>+ /// The major version of clang (corresponds to
CLANG_VERSION_MAJOR)</div>
<div>+ unsigned Major;</div>
<div>+ /// The minor version of clang (corresponds to
CLANG_VERSION_MINOR)</div>
<div>+ unsigned Minor;</div>
<div>+};</div>
</div>
</blockquote>
<div><br>
</div>
<div>VersionTuple is a bit overkill here, but do we really need
another version struct?</div>
</blockquote>
<br>
My main rationale for making a new version struct was that this
would be effectively ABI-frozen. Since the version code is used to
decide whether or not to attempt to use the plugin, an ABI-mismatch
is particularly deadly for the compiler in this code.<br>
<blockquote cite="mid:EF146988-CF53-4A2C-8194-D17CC7DA1E2F@apple.com" type="cite">
<blockquote type="cite">
<div></div></blockquote></blockquote></div></blockquote><div><br></div><div>Fair enough. I doubt VersionTuple is changing any time soon, but you make a good point.</div><br><br><blockquote type="cite"><div bgcolor="#FFFFFF" text="#000000"><blockquote cite="mid:EF146988-CF53-4A2C-8194-D17CC7DA1E2F@apple.com" type="cite"><blockquote type="cite"><div><div>+ /// Arguments to the plugin</div>
<div>+ SmallVector<std::pair<std::string,
std::string>, 4> Args;</div>
</div>
</blockquote>
<div><br>
</div>
<div>Should this just be a std::vector? I think the most common
case will be 0 args, in which case SmallVector doesn't do much
good.</div>
</blockquote>
<br>
I originally had this as a std::vector, and was told to make it a
SmallVector instead.<br></div></blockquote><div><br></div><div>Ha, okay.</div><div><br></div><div>Jordan</div></div><div><br></div></body></html>