Michael Spencer
2014-Jun-02 20:05 UTC
[LLVMdev] [lld] LLD's software architecture (update)
On Mon, Jun 2, 2014 at 12:49 AM, Rui Ueyama <ruiu at google.com> wrote:> I agree to move these files to Core. Any objections?None here. - Michael Spencer> > On Mon, Jun 2, 2014 at 12:44 AM, Bas van den Berg > <b.van.den.berg.nl at gmail.com> wrote: >> >> The inverted dependency of Core to ReaderWriter via Simple.h was already >> present. >> My idea was to fix the problem before it gets bigger. >> My proposal would be to move Simple.h and Alias.h to Core. Similar to >> UndefinedAtom.h etc. >> It would be even nicer to make the naming consistent as well, since there >> already is >> UndefinedAtom.h >> SharedLibaryAtom.h >> etc >> >> Maybe: >> SimpleAtom.h >> AliasAtom.h >> >> Moving the file to core fixes the dependencies. >> >> >> >> >> On Mon, Jun 2, 2014 at 9:25 AM, Rui Ueyama <ruiu at google.com> wrote: >>> >>> On Mon, Jun 2, 2014 at 12:01 AM, Bas van den Berg >>> <b.van.den.berg.nl at gmail.com> wrote: >>>> >>>> Hi, >>>> >>>> I've been following the changes in LLD's software architecture. >>>> Recently a new file was added: Alias.h >>>> In this file, the curret Atom set is extended with an AliasAtom. >>>> While the change seems innocent enough, it has some nasty >>>> potential. Simple.h and Alias.h are placed in the ReaderWriter >>>> component. This is the 2nd component in the layering: >>>> Driver >>>> ReaderWriter >>>> Passes >>>> Core >>>> >>>> The problem is that Core depends on these files. So in my opinion, >>>> they belong to that Component. The greater impact here is that >>>> once core starts depending on ReaderWriter, architecture Erosion >>>> will happen. This would obviously not be a good thing, since the >>>> current design is fresh and clean. >>> >>> >>> Core's dependency to Simple.h was there before the addition of Alias.h, >>> so it's not new. And I don't feel like it's that bad as you might be >>> feeling. Do you have any suggestion to get rid of it? Just moving the file >>> to Core? >>> >>>> I would love to supply patches, but am having issues getting >>>> lld to build under Ubuntu 12.04. (but that's another story ;) ). >>>> >>>> I've posted the updated architecture view here: >>>> http://www.c2lang.org/docs/lld_architecture_20140602.png >>>> >>>> >>>> _______________________________________________ >>>> LLVM Developers mailing list >>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >>>> >>> >> > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >
I will create a patch to move the files and send it for review. On Mon, Jun 2, 2014 at 1:05 PM, Michael Spencer <bigcheesegs at gmail.com> wrote:> On Mon, Jun 2, 2014 at 12:49 AM, Rui Ueyama <ruiu at google.com> wrote: > > I agree to move these files to Core. Any objections? > > None here. > > - Michael Spencer > > > > > On Mon, Jun 2, 2014 at 12:44 AM, Bas van den Berg > > <b.van.den.berg.nl at gmail.com> wrote: > >> > >> The inverted dependency of Core to ReaderWriter via Simple.h was already > >> present. > >> My idea was to fix the problem before it gets bigger. > >> My proposal would be to move Simple.h and Alias.h to Core. Similar to > >> UndefinedAtom.h etc. > >> It would be even nicer to make the naming consistent as well, since > there > >> already is > >> UndefinedAtom.h > >> SharedLibaryAtom.h > >> etc > >> > >> Maybe: > >> SimpleAtom.h > >> AliasAtom.h > >> > >> Moving the file to core fixes the dependencies. > >> > >> > >> > >> > >> On Mon, Jun 2, 2014 at 9:25 AM, Rui Ueyama <ruiu at google.com> wrote: > >>> > >>> On Mon, Jun 2, 2014 at 12:01 AM, Bas van den Berg > >>> <b.van.den.berg.nl at gmail.com> wrote: > >>>> > >>>> Hi, > >>>> > >>>> I've been following the changes in LLD's software architecture. > >>>> Recently a new file was added: Alias.h > >>>> In this file, the curret Atom set is extended with an AliasAtom. > >>>> While the change seems innocent enough, it has some nasty > >>>> potential. Simple.h and Alias.h are placed in the ReaderWriter > >>>> component. This is the 2nd component in the layering: > >>>> Driver > >>>> ReaderWriter > >>>> Passes > >>>> Core > >>>> > >>>> The problem is that Core depends on these files. So in my opinion, > >>>> they belong to that Component. The greater impact here is that > >>>> once core starts depending on ReaderWriter, architecture Erosion > >>>> will happen. This would obviously not be a good thing, since the > >>>> current design is fresh and clean. > >>> > >>> > >>> Core's dependency to Simple.h was there before the addition of Alias.h, > >>> so it's not new. And I don't feel like it's that bad as you might be > >>> feeling. Do you have any suggestion to get rid of it? Just moving the > file > >>> to Core? > >>> > >>>> I would love to supply patches, but am having issues getting > >>>> lld to build under Ubuntu 12.04. (but that's another story ;) ). > >>>> > >>>> I've posted the updated architecture view here: > >>>> http://www.c2lang.org/docs/lld_architecture_20140602.png > >>>> > >>>> > >>>> _______________________________________________ > >>>> LLVM Developers mailing list > >>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > >>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >>>> > >>> > >> > > > > > > _______________________________________________ > > LLVM Developers mailing list > > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140602/f7d7f49c/attachment.html>
Bas van den Berg
2014-Jul-10 07:08 UTC
[LLVMdev] [lld] LLD's software architecture (update)
I've run a new scan of LLD's architecture and it appears to be getting cleaner! There are still some upward dependencies however: - core depends on Reader and Writer - passes depend on Reader and Writer - ReaderWriter/PECOFF/ReaderCOFF.cpp depends on Driver.h The updated architecture can be seen here: http://www.c2lang.org/docs/lld_architecture_20140710.png On Tue, Jun 3, 2014 at 3:07 AM, Rui Ueyama <ruiu at google.com> wrote:> I will create a patch to move the files and send it for review. > > > On Mon, Jun 2, 2014 at 1:05 PM, Michael Spencer <bigcheesegs at gmail.com> > wrote: > >> On Mon, Jun 2, 2014 at 12:49 AM, Rui Ueyama <ruiu at google.com> wrote: >> > I agree to move these files to Core. Any objections? >> >> None here. >> >> - Michael Spencer >> >> > >> > On Mon, Jun 2, 2014 at 12:44 AM, Bas van den Berg >> > <b.van.den.berg.nl at gmail.com> wrote: >> >> >> >> The inverted dependency of Core to ReaderWriter via Simple.h was >> already >> >> present. >> >> My idea was to fix the problem before it gets bigger. >> >> My proposal would be to move Simple.h and Alias.h to Core. Similar to >> >> UndefinedAtom.h etc. >> >> It would be even nicer to make the naming consistent as well, since >> there >> >> already is >> >> UndefinedAtom.h >> >> SharedLibaryAtom.h >> >> etc >> >> >> >> Maybe: >> >> SimpleAtom.h >> >> AliasAtom.h >> >> >> >> Moving the file to core fixes the dependencies. >> >> >> >> >> >> >> >> >> >> On Mon, Jun 2, 2014 at 9:25 AM, Rui Ueyama <ruiu at google.com> wrote: >> >>> >> >>> On Mon, Jun 2, 2014 at 12:01 AM, Bas van den Berg >> >>> <b.van.den.berg.nl at gmail.com> wrote: >> >>>> >> >>>> Hi, >> >>>> >> >>>> I've been following the changes in LLD's software architecture. >> >>>> Recently a new file was added: Alias.h >> >>>> In this file, the curret Atom set is extended with an AliasAtom. >> >>>> While the change seems innocent enough, it has some nasty >> >>>> potential. Simple.h and Alias.h are placed in the ReaderWriter >> >>>> component. This is the 2nd component in the layering: >> >>>> Driver >> >>>> ReaderWriter >> >>>> Passes >> >>>> Core >> >>>> >> >>>> The problem is that Core depends on these files. So in my opinion, >> >>>> they belong to that Component. The greater impact here is that >> >>>> once core starts depending on ReaderWriter, architecture Erosion >> >>>> will happen. This would obviously not be a good thing, since the >> >>>> current design is fresh and clean. >> >>> >> >>> >> >>> Core's dependency to Simple.h was there before the addition of >> Alias.h, >> >>> so it's not new. And I don't feel like it's that bad as you might be >> >>> feeling. Do you have any suggestion to get rid of it? Just moving the >> file >> >>> to Core? >> >>> >> >>>> I would love to supply patches, but am having issues getting >> >>>> lld to build under Ubuntu 12.04. (but that's another story ;) ). >> >>>> >> >>>> I've posted the updated architecture view here: >> >>>> http://www.c2lang.org/docs/lld_architecture_20140602.png >> >>>> >> >>>> >> >>>> _______________________________________________ >> >>>> LLVM Developers mailing list >> >>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> >>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> >>>> >> >>> >> >> >> > >> > >> > _______________________________________________ >> > LLVM Developers mailing list >> > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> > >> > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140710/8940827b/attachment.html>