[PATCH] D91351: [tooling] Implement determinsitic ordering of CompilationDatabasePlugins

[Nathan Ridge via Phabricator via cfe-commits]

nridge accepted this revision.
nridge added a comment.
This revision is now accepted and ready to land.

Not sure if I'm authorized to approve changes to libTooling, but this LGTM!



================
Comment at: clang/lib/Tooling/CompilationDatabase.cpp:73
        CompilationDatabasePluginRegistry::entries()) {
+    Plugins.emplace_back(Database.getName(), Database.instantiate());
+  }
----------------
njames93 wrote:
> nridge wrote:
> > A side effect of this change is that every plugin will get instantiated, not just the ones for which we attempt a load.
> > 
> > Perhaps we should store a pointer to the entry in the vector here, and instantiate() in the second loop below?
> The original setup was to instantiate every plugin until we find one that is able to load from the directory.
> As for your proposed solution, that wouldn't work as we can't get the priority without instantiating the plugin.
> The current registry system also doesn't lend itself nicely.
> I'm not too familiar, but it could be possible to specialise `SimpleRegistryEntry` for `CompilationDatabasePlugin`.
> that wouldn't work as we can't get the priority without instantiating the plugin

Right, of course, I overlooked that.

Anyways, it looks like instantiating the plugin just calls its constructor and the constructors of both existing plugins are no-ops, so it shouldn't be a concern.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91351/new/

https://reviews.llvm.org/D91351