<div style="font-family: arial, helvetica, sans-serif"><font size="2"><div>General style question: most of the variables / params are lower case first letter... I assume there's a reason to do this here (as the style guide contradicts)?</div>
<div><br></div><div>+  bool clang_plugin_init(int argc, plugin::PluginArg *argv,</div>
<div>+      plugin::Version *version) {</div><div>+    // Check the version of clang</div><div>+    if (version-&gt;major != CLANG_VERSION_MAJOR ||</div>
<div>+        version-&gt;minor != CLANG_VERSION_MINOR)</div><div>+      return false;</div><div>+  </div><div>+    for (int i = 0; i &lt; argc; ++i) {</div><div>+      if (!strcmp(argv[i].name, "-some-arg")) {</div>


<div>+        // Handle the command line argument</div><div>       }</div><div>     }</div><div>     return true;</div><div>   }</div><div><br></div><div>So if the plugin needs data from the arguments, the idea is to put it into global variables?</div>


<div>My aesthetic sense is offended by the fact that my pragmatism thinks that for plugins which are inherently static data in the shared object file this is probably ok.</div><div><br></div><div><div>+struct PluginFileCallbacks {</div>

<div>+  /// A callback that allows plugins to view (but not modify) the AST of a file.</div><div>+  ASTConsumer *astConsumer;</div><div>+  /// A callback that allows plugins to be notified of preprocessor directives.</div>

<div>+  PPCallbacks *ppCallback;</div><div>+  /// A callback that allows plugins to be notified of warnings and errors.</div><div>+  DiagnosticConsumer *diagnostics;</div><div>+};</div></div><div><br></div><div>Yay! :)</div>

<div><br></div><div><div>+  // Parse the plugin arguments</div><div>+  std::vector<std::pair<std::string, std::string> > ParsedPluginArgs;</div><div>+  for (arg_iterator it = Args.filtered_begin(OPT_fplugin_arg),</div>

<div>+      end = Args.filtered_end(); it != end; ++it) {</div></div><div><br></div><div>Is this indent intentional?</div><div><br></div><div><div>+  // Add the plugin arguments to a parsedPluginArgs</div></div><div><br>
</div><div>Why the lower case 'p' here?</div><div><br></div><div><div>+  // Add the plugin arguments to a parsedPluginArgs</div><div>+  for (int i = 0, e = Opts.CompilerPlugins.size(); i != e; ++i) {</div><div>+    std::string base = llvm::sys::path::stem(Opts.CompilerPlugins[i]);</div>
<div>+    for (std::vector<std::pair<std::string, std::string> >::iterator</div><div>+           it = ParsedPluginArgs.begin(), end = ParsedPluginArgs.end();</div><div>+           it != end; ++it) {</div><div>
+      if (it->first == base) {</div><div><br></div><div>I assume the 'else' part are compiler options that we didn't find a plugin for - don't we want some kind of indication that this is not used, or is this done elsewhere and I'm missing it?</div>
<div><br></div><div>+        std::string argpair = it->second;</div></div><div><br></div><div>I find it slightly confusing that this is called 'pair', when it's a string. Perhaps call it argAssignment or something?</div>
<div><br></div><div><div>+  // The wrapper action that manages all plugin invocations. This is what we use</div><div>+  // as the main FrontendAction for the CompilerInstance.</div><div>+  class PluginWrapperAction : public WrapperFrontendAction {</div>
</div><div><br></div><div>Have I mentioned that I really like this design? :)</div><div><br></div><div><div>+    clang::plugin::PluginFileCallbacks *callbacks = it->getCallbacks();</div><div>+    memset(callbacks, 0, sizeof(clang::plugin::PluginFileCallbacks));</div>
</div><div><br></div><div>Any reason not to give PluginFileCallbasks a constructor that 0-initializes instead?</div><div><br></div><div>Cheers,</div><div>/Manuel</div><div><br></div><div><br></div><div><br></div><div><br>
</div>
</font></div>