On Fri, May 18, 2018 at 2:22 PM Eric Liu <ioe...@google.com> wrote: > Thanks a lot for looking into this Bruno! The fix looks promising; I'll > give it a try next week :D > > On Fri, May 18, 2018 at 10:33 PM David Blaikie via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> I haven't looked in detail here, but in general: Don't split an >> implementation and its headers into different notional libraries, as that >> breaks modular code generation (an implementation and its headers usually >> have circular dependencies - inline functions that call non-inline >> functions, and the other way around & so if they're in two different >> libraries they won't be able to link (due to the way unix linkers >> work/dependencies are resolved in a single pass, never looping back around). >> > David, I'm not exactly sure if I understand... This change pulls both > declarations and implementations into a self-contained library which could > be shared by clang-format and other tools that manipulate #includes. This > seems to be a normal refactoring, and I hope this doesn't break modular > code generation. >
Sounds OK - so long as both header and implementation go together is all. (by the name "Inclusions" I was worried maybe just the headers had been pulled out, but not their implementation) > >> On Fri, May 18, 2018 at 1:10 PM Bruno Cardoso Lopes via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> >>> >>> >>> On Fri, May 18, 2018 at 12:46 PM Bruno Cardoso Lopes < >>> bruno.card...@gmail.com> wrote: >>> >>>> >>>> >>>> On Fri, May 18, 2018 at 11:54 AM Vedant Kumar via cfe-commits < >>>> cfe-commits@lists.llvm.org> wrote: >>>> >>>>> On May 18, 2018, at 11:48 AM, Eric Liu <ioe...@google.com> wrote: >>>>> >>>>> >>>>> So I have reverted this with r332751. >>>>> >>>>> >>>>> Thanks! >>>>> >>>>> >>>>> I can't see how this introduced cyclic dependencies in module build, >>>>> as the dependencies should be clangTooling -> clangFormat -> >>>>> clangToolingInclusions. I'm wondering if there is any module >>>>> configurations >>>>> that I need to update to make this work. Right now, module doesn't seem to >>>>> make any difference between clangTooling and clangToolingInclusions... >>>>> I'd appreciate it if someone who knows how clang module build is set up >>>>> could help take a look. >>>>> >>>>> >>>>> + Bruno & David who have more experience in this area than I do. >>>>> >>>> >>>> Gonna try to reproduce and take a look! >>>> >>> >>> I could reproduce it. You should be good to go if you add another top >>> level module for Inclusions (and break the dep): >>> >>> --- a/include/clang/module.modulemap >>> +++ b/include/clang/module.modulemap >>> @@ -153,3 +153,8 @@ module Clang_ToolingCore { >>> requires cplusplus >>> umbrella "Tooling/Core" module * { export * } >>> } >>> + >>> +module Clang_ToolingInclusions { >>> + requires cplusplus >>> + umbrella "Tooling/Inclusions" module * { export * } >>> +} >>> >>> -- >>> Bruno Cardoso Lopes >>> http://www.brunocardoso.cc >>> _______________________________________________ >>> cfe-commits mailing list >>> cfe-commits@lists.llvm.org >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits