[cfe-commits] [PATCH] Implement a better plugin API for clang

[Manuel Klimek]

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)?

+  bool clang_plugin_init(int argc, plugin::PluginArg *argv,
+      plugin::Version *version) {
+    // Check the version of clang
+    if (version->major != CLANG_VERSION_MAJOR ||
+        version->minor != CLANG_VERSION_MINOR)
+      return false;
+
+    for (int i = 0; i < argc; ++i) {
+      if (!strcmp(argv[i].name, "-some-arg")) {
+        // Handle the command line argument
       }
     }
     return true;
   }

So if the plugin needs data from the arguments, the idea is to put it into
global variables?
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.

+struct PluginFileCallbacks {
+  /// A callback that allows plugins to view (but not modify) the AST of a
file.
+  ASTConsumer *astConsumer;
+  /// A callback that allows plugins to be notified of preprocessor
directives.
+  PPCallbacks *ppCallback;
+  /// A callback that allows plugins to be notified of warnings and errors.
+  DiagnosticConsumer *diagnostics;
+};

Yay! :)

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

Is this indent intentional?

+  // Add the plugin arguments to a parsedPluginArgs

Why the lower case 'p' here?

+  // Add the plugin arguments to a parsedPluginArgs
+  for (int i = 0, e = Opts.CompilerPlugins.size(); i != e; ++i) {
+    std::string base = llvm::sys::path::stem(Opts.CompilerPlugins[i]);
+    for (std::vector<std::pair<std::string, std::string> >::iterator
+           it = ParsedPluginArgs.begin(), end = ParsedPluginArgs.end();
+           it != end; ++it) {
+      if (it->first == base) {

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?

+        std::string argpair = it->second;

I find it slightly confusing that this is called 'pair', when it's a
string. Perhaps call it argAssignment or something?

+  // The wrapper action that manages all plugin invocations. This is what
we use
+  // as the main FrontendAction for the CompilerInstance.
+  class PluginWrapperAction : public WrapperFrontendAction {

Have I mentioned that I really like this design? :)

+    clang::plugin::PluginFileCallbacks *callbacks = it->getCallbacks();
+    memset(callbacks, 0, sizeof(clang::plugin::PluginFileCallbacks));

Any reason not to give PluginFileCallbasks a constructor that 0-initializes
instead?

Cheers,
/Manuel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120622/5b6a2c92/attachment.html>