[PATCH] Introduce a ModuleProvider interface (NFC)

[Richard Smith]

On Tue, Jun 16, 2015 at 10:15 AM, Adrian Prantl <aprantl at apple.com> wrote:

> Hi Richard,
>
> based on an earlier discussion on cfe-commits, here is an NFC patch that
> introduces a ModuleProvider interface to handle the physical storage of
> clang modules. The primary motivation for this is module debugging. The
> idea is that CodeGen will implement this interface to wrap clang modules in
> an object file container. Other users of libclang that do not care about
> debug info can use the default implementation provided in this patch and
> won’t need to link against CodeGen and LLVM.
>
> The majority of the patch consists of threading the new interface through
> everywhere, the interesting bits are in AST/ModuleProvider.h and
> Lex/ModuleLoader.h. There are no changes necessary to clang-tools-extra.
>
> -- adrian
>
>
>
> Introduce a ModuleProvider interface (NFC).
>
> A ModuleProvider provides an interface handling the physical storage of
> Clang modules. The default implementation is SimpleModuleProvider, which
> uses a flat file for the output.
>

I don't agree that that's what this patch does -- I think your
ModuleProvider provides an interface abstracting a wrapper file format
containing a serialized AST bitstream.

I think that doesn't fit very well in ModuleLoader, which should support
providing a module by means not involving an AST serialized in our normal
bitcode format (for instance, converting it from DWARF, or loading some
standardized serialized AST format, or converting from another language, or
whatever). Can you store the ModuleProvider at a higher level (somewhere
where we know we're dealing with a bitstream file, such as the
CompilerInstance)?


The main application for this interface will be an implementation that
> uses LLVM to wrap the module in an ELF/Mach-O/COFF container to store
> debug info alongside the module.
>

+++ b/include/clang/AST/ModuleProvider.h

Does this really belong in libAST? That seems like a poor fit to me. (Also,
I'm not really happy with the name, since this thing does not provide
modules in any meaningful sense.)


+  /// \brief Return an ASTconsumer that can be chained with a

Miscapitalization.


+  virtual std::unique_ptr<ASTConsumer> CreateModuleContainerGenerator(
+      DiagnosticsEngine &Diags, const std::string &ModuleName,
+      const HeaderSearchOptions &HSO, const PreprocessorOptions &PPO,
+      const CodeGenOptions &CGO, const TargetOptions &TO, const
LangOptions &LO,
+      llvm::raw_pwrite_stream *OS,
+      std::shared_ptr<ModuleBuffer> Buffer) const = 0;

This interface seems very specific to generating debug information for a
module. Should the computation of these options structs (when they're
needed) live inside the ModuleProvider subclass rather than being provided
by the caller of this function?


+++ b/include/clang/Lex/ModuleLoader.h
-  /// \brief Attempt to load the given module.
+/// \brief Attempt to load the given module.

Broken indentation here.


+/// \brief A shared pointer to a ModuleProvider that is non-null.
+class SharedModuleProvider : public std::shared_ptr<ModuleProvider> {
+  SharedModuleProvider() = delete;
+  SharedModuleProvider(ModuleProvider *MP);
+public:
+  template<class T, typename... ArgTypes>
+  static SharedModuleProvider Create(ArgTypes &&... Args) {
+    return SharedModuleProvider(new T(std::forward<ArgTypes>(Args)...));
+  }
+};

Is it really worth having a separate class for this? Your Create function
seems worse than using make_shared directly (it'll result in two
allocations rather than one).


+++ b/include/clang/Serialization/GlobalModuleIndex.h
+  /// \param MP The Moduleprovider to be used to unwrap the modules.

Miscapitalization.


+++ b/lib/Frontend/FrontendActions.cpp
+  // The debug info emitted by ModuleContainerGenerator is not affected by
the
+  // optimization level.

... so why bother explicitly setting it? The default constructor already
set it to 0.


 GeneratePCHAction::CreateASTConsumer(CompilerInstance &CI, StringRef
InFile) {
...
+
 Consumers.push_back(CI.getModuleProvider().CreateModuleContainerGenerator(

Are you intending to put debug information into PCH files too, not just
into modules?


+++ b/lib/Frontend/MultiplexConsumer.cpp

Please commit these changes separately; it's sufficient to mention in your
commit message that your subsequent change will add test coverage.


+++ b/tools/c-index-test/c-index-test.c

Please commit this typo fix separately.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150616/ada5acb3/attachment.html>