Thanks! LGTM + "Supported options are `raw' and `obj'.">;
We don't typically use backticks for quotation. On Thu, Jul 16, 2015 at 4:41 PM, Adrian Prantl <[email protected]> wrote: > > On Jul 15, 2015, at 2:02 PM, Richard Smith <[email protected]> wrote: > > On Wed, Jul 15, 2015 at 1:51 PM, Adrian Prantl <[email protected]> wrote: > >> >> On Jul 15, 2015, at 1:16 PM, Richard Smith <[email protected]> wrote: >> >> On Wed, Jul 15, 2015 at 12:54 PM, Adrian Prantl <[email protected]> >> wrote: >> >>> Here is a patch that implements a -fmodule-format=[obj,raw] option. I >>> have not yet implemented adding the module format to the module hash or >>> filename. >>> >> >> Generally, we only have a -cc1 flag to switch to the non-default state. >> >> >> Okay I think it’s better then to bake the default into cc1 rather than >> the driver. >> >> >> >>> The default setting is obj (based on the assumption that this is most >>> beneficial to platforms with a shared module cache) and the driver adds an >>> explicit -fmodule-format=raw on Linux. >>> >> >> I think this is backwards; I think putting more stuff into the >> precompiled module format should be opt-in rather than opt-out. >> >> I am not sure whether having the driver emit this option for Linux is a >>> good idea: Are explicit module builds a feature of the Google build system, >>> or are they a Linux platform feature? >>> >> >> Neither; they're a Clang feature. Implicit module builds are a >> compatibility feature for legacy build systems, and should be avoided >> wherever possible because they introduce a performance hit, interact poorly >> with distributed builds, behave badly if the cache gets cleared (especially >> once we put debug info in modules), and so on. >> >> Currently the only way to enable obj-wrapped modules on Linux is to pass >>> a cc1 option. Even with -fmodule-format=raw specified, clang can still read >>> obj-wrapped modules. >>> >> >> OK, but presumably we'll add -gmodules or similar at some point to >> resolve that issue. >> >> >> Yes. >> >> My updated plan is to make =raw the default. The driver expands >> “-gmodules" into something like "-dwarf-ext-typerefs[TBD] >> -fmodule-format=obj”. Having =raw be the default is safe as long as we >> encode the module format into the module hash so the raw and obj versions >> don’t conflict. Once we have a pass to translate module ast->obj as you >> would do for explicit builds, we can make more fine-grained adjustments to >> this. >> >> >> I’m not in love with the actual implementation, so suggestions and >>> feedback are very welcome! >>> >> >> I assume the "if (1 || ..." was not intentional? >> >> >> Yes :-) I had that in there to run the testsuite with the different >> default settings. >> >> >> It doesn't seem ideal to have the top-level driver create the >> wrapper-format handler, and then ignore that from the frontend code. That's >> also not going to scale to module formats other than obj and raw. Is there >> any other way we can deal with this without breaking the layering? >> >> >> That’s exactly what bothered me. The wrapper-format handler is primarily >> there so Frontend does not need to depend on CodeGen. Currently we’re >> passing it in to CompilerInstance at creation time (which happens at the >> very top level). It might be possible to solve this a little more elegantly >> by having a >> >> class PCHContainerOperationsRegistry { >> void registerPCHContainerOperations(PCHContainerOperations *Handler); >> shared_ptr<PCHContainerOperations> getPCHContainerOperations(PCHFormat >> Format); >> } >> >> have the toplevel register RawPCHContainerOperations and >> ObjectFilePCHContainerOperations and have >> CompilerInstance::getPCHContainerOperations() return the appropriate >> handler for the currently registered CodeGenOptions (since the caller >> usually doesn’t have access to the CodeGenOptions). >> > > Yes, something along those lines would make sense to me. I was thinking of > a slightly different design, more like > > class ASTContainerFormatHandler { > PCHContainerOperations *getFormat(StringRef Format) = 0; > }; > > ... but I'll leave the details up to you. Maybe this would also be a good > time to think about the writer / reader split (for consumers that want to > read object-file-wrapped ASTs but don't want to link against the LLVM code > generator)? > > > This updated patch > - introduces -fmodule-format=[format] > - defaults to raw containers > - supports arbitrary module container formats that libclang is agnostic to > - adds the format to the module hash > - splits the old PCHContainerOperations into PCHContainerWriter and > PCHContainerReader. > The module format string is now part of HeaderSearchOptions instead of > CodeGenOptions. > > -- adrian > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
