On 03/19/2015 09:38 AM, David Blaikie wrote:> > > On Thu, Mar 19, 2015 at 9:34 AM, Reed Kotler <reed.kotler at imgtec.com > <mailto:reed.kotler at imgtec.com>> wrote: > > On 03/19/2015 09:24 AM, David Blaikie wrote: >> >> >> On Thu, Mar 19, 2015 at 9:18 AM, Reed Kotler >> <reed.kotler at imgtec.com <mailto:reed.kotler at imgtec.com>> wrote: >> >> Well, you are an mclinker contributor >> >> >> Me personally? Not that I know of. > Sorry. I thought i had seen your name in an mclinker commit. >> >> and Google uses mclinker >> >> >> So I've been told, though I hadn't even heard of mclinker until >> this email thread. > It's another linker (we don't have enough of them :) ) . It's used > to run on mobile devices > and has been designed with that criteria in mind. (At least that > is my understanding). > >> and now it's broken as the result of your change. >> >> >> This is a pretty standard state of affairs for LLVM-using >> projects, LLVM's APIs change continuously. > Probably then this is a configuration issue I need to take up with > mclinker folks. > Their build instructions have you just download tip of tree from > svn and that clearly is not guaranteed to work. > Probably their wiki should list the version from LLVM to download. > > > I don't know what the deal is with mclinker's strategy here, but some > out of tree projects follow llvm - the nature of that is that it'll > break from time to time (just as in-tree projects break with LLVM API > changes (not many people checkout/build some subprojects like polly, > etc)) - for projects that follow ToT, not any particular snapshot > revision, it's just a matter of who syncs up first & sees the break - > everyone usually takes some responsibility to fix things up if they > break. (and/or loop the right people in if you sync up & it's broken > and you don't know how to fix it & have a chat about how to fix it)I have to confess ignorance here because I'm just following their wiki instructions which are written as if it's TOT compatible with LLVM.> > > >> I still don't see any justification to making a change in a >> public interface that is used by other non LLVM projects >> to fix some issue with clang warnings. People should be able >> to derive from those classes. I can't understand >> your reasoning as to why these classes must be final. >> >> >> Because if they're not, it's easy to write bugs with them - >> especially in non-tree projects, strangely enough. (if you derive >> from this type, then happen to own those derived objects >> polymorphically, you'll invoke undefined behavior - by making the >> type 'final' it removes the ability to write such bugs). > Isn't that a problem with the design of this class? (or these > classes)? > > > Not really - not for LLVM's use cases (which are the ones the API is > designed for, of course). We could make the dtor virtual instead - but > that's unnecessary overhead compared to just ensuring (by use of > protected dtors and final classes) that the class can't be destroyed > polymorphically anyway. It's a legitimate design to just disallow it > rather than adding overhead to allow it when it's not needed by the > use cases we have.Is it really necessary to worry about the overhead of virtual destructors for command line processing classes? It makes the classes less generally useful for no measurable benefit.> > >> I was kind of surprised that there are no mclinker build bots >> that would have picked this up right away. >> >> >> On 03/19/2015 09:08 AM, David Blaikie wrote: >>> >>> >>> On Thu, Mar 19, 2015 at 9:05 AM, Reed Kotler >>> <reed.kotler at imgtec.com <mailto:reed.kotler at imgtec.com>> wrote: >>> >>> On 03/19/2015 08:55 AM, David Blaikie wrote: >>>> >>>> >>>> On Thu, Mar 19, 2015 at 4:30 AM, Reed Kotler >>>> <Reed.Kotler at imgtec.com >>>> <mailto:Reed.Kotler at imgtec.com>> wrote: >>>> >>>> One could argue that mclinker is doing something >>>> good or not by how it's using this class >>>> but I don't see the need for parser<bool> to be >>>> final. That is a subjective opinion that mclinker >>>> needs to >>>> be changed. >>>> >>>> I think that "final" was added to some of these >>>> command line classes to avoid some kind of clang >>>> warning. >>>> >>>> That seems wrong to me that the tools are dictating >>>> something in an API. >>>> >>>> >>>> Well the warning's a pretty good one - and dictates a >>>> way to design an API safely to avoid the possibility of >>>> a bad delete. >>>> >>>> It's not uncommon to have a polymorphic hierarchy that >>>> doesn't require polymorphic ownership. To ensure that >>>> polymorphic ownership doesn't sneak in, we have a >>>> warning to ensure the class either has an interface >>>> compatible with polymorphic ownership (has a virtual >>>> dtor) or disallows polymorphic ownership (has a >>>> protected dtor in any non-final base). >>>> >>>> There is a narrower warning that only diagnoses this at >>>> the use of polymorphic destruction >>>> (-Wdelete-non-virtual-dtor) but we've not needed to >>>> fallback to that in LLVM so far & I'd mildly rather >>>> avoid doing so. The protected-final-or-virtual pattern >>>> seems fairly elegant/self-documenting to me. >>>> >>>> There may be other valid reasons to create derived >>>> classes. >>>> >>>> I think we should remove the "final" attribute on >>>> those classes and fix the clang warning issue in some >>>> better way than changing the API. >>>> >>>> If mclinker had been part of the llvm project tools >>>> tree that would not have been allowed even. >>>> >>>> >>>> Or we might've decided that the use in mclinker was bad >>>> & should be fixed - I haven't really looked at it to >>>> consider the details of its use, to be honest. >>> There are lots of valid reasons to want to be able to >>> derive classes; that's why C++ lets you do that. :) >>> >>> I don't think that LLVM judging MCLinker coding is >>> relevant here. >>> >>> >>> It seems like it is to me - when refactoring code we look at >>> the use cases and judge what the right way to handle those >>> use cases is. One of those ways is to fix/change/remove the >>> use-cases that seem problematic/unnecessary/incorrect. >>> >>> Lots of projects use LLVM now and changing the API in >>> this way >>> does not have justification other than to deal with some >>> clang warning issues that can be solved in other ways. >>> >>> >>> I think the practices that the clang warning >>> enforces/encourages are good ones, otherwise I would've >>> fixed/changed/disabled the warning. We've used these >>> practices/this warning for a while now & it seems to suit >>> LLVM's use cases pretty well so far. >>> >>> I don't see any justification for deciding once and for >>> all that there are no valid reasons for deriving from >>> these classes >>> and to use a c++ feature to enforce that. >>> >>> As I said earlier, had mclinker been in the tools >>> subdirectory of LLVM then the change the CommandLine.h >>> would not have even been allowed. >>> >>> >>> >>> >>>> >>>> ________________________________________ >>>> From: Simon Atanasyan [simon at atanasyan.com >>>> <mailto:simon at atanasyan.com>] >>>> Sent: Wednesday, March 18, 2015 9:35 PM >>>> To: Reed Kotler >>>> Cc: David Blaikie; Simon Atanasyan; Rich Fuhler; >>>> llvmdev at cs.uiuc.edu <mailto:llvmdev at cs.uiuc.edu> >>>> Subject: Re: [LLVMdev] Final added to parser<bool> >>>> >>>> Hi Reed, >>>> >>>> The FalseParser has a rather strange design. It's >>>> purpose to parse >>>> options like -no-to-do-something and always return >>>> false even if a >>>> user provides -no-to-do-something=no. That should >>>> be fixed on the >>>> MCLinker side. >>>> >>>> On Thu, Mar 19, 2015 at 5:14 AM, reed kotler >>>> <rkotler at mips.com <mailto:rkotler at mips.com>> wrote: >>>> > >>>> //===----------------------------------------------------------------------===// >>>> > // FalseParser >>>> > >>>> //===----------------------------------------------------------------------===// >>>> > class FalseParser : public parser<bool> { >>>> > public: >>>> > explicit FalseParser(Option &O) : >>>> parser<bool>(O) { } >>>> > >>>> > // parse - Return true on error. >>>> > bool parse(cl::Option& O, StringRef ArgName, >>>> StringRef Arg, bool& Val) { >>>> > if (cl::parser<bool>::parse(O, ArgName, Arg, >>>> Val)) >>>> > return false; >>>> > Val = false; >>>> > return false; >>>> > } >>>> > }; >>>> > >>>> > I don't know the history of this. I'm just >>>> starting to do some mclinker work >>>> > to add the new mips r6 relocations to it. >>>> > >>>> > >>>> > On 03/18/2015 07:00 PM, David Blaikie wrote: >>>> > >>>> > >>>> > >>>> > On Wed, Mar 18, 2015 at 6:48 PM, reed kotler >>>> <rkotler at mips.com <mailto:rkotler at mips.com>> wrote: >>>> >> >>>> >> Hi David, >>>> >> >>>> >> Is there a reason that we need to have "final" >>>> for parser<bool> ??? >>>> > >>>> > >>>> > Clang has a (reasonable) warning for types with >>>> virtual functions and a >>>> > non-virtual dtor. This warning is suppressed if >>>> the dtor is protected or the >>>> > class is final (since in the first case it's >>>> clear that the user intends not >>>> > to destroy objects via base pointers, only >>>> derived ones - and in the second >>>> > case there's no risk of derived classes, so >>>> public access to the dtor is >>>> > safe even without virtual dispatch. >>>> > >>>> > Since the parser hierarchy never needed >>>> polymorphic destruction (all >>>> > instances are concrete instances of derived >>>> classes owned and destroyed >>>> > directly, not via base pointers) this seemed like >>>> a fine way to structure >>>> > the API. >>>> >>>> -- >>>> Simon Atanasyan >>>> >>>> >>> >>> >> >> > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150319/137bed44/attachment.html>
On Thu, Mar 19, 2015 at 9:52 AM, Reed Kotler <reed.kotler at imgtec.com> wrote:> On 03/19/2015 09:38 AM, David Blaikie wrote: > > > > On Thu, Mar 19, 2015 at 9:34 AM, Reed Kotler <reed.kotler at imgtec.com> > wrote: > >> On 03/19/2015 09:24 AM, David Blaikie wrote: >> >> >> >> On Thu, Mar 19, 2015 at 9:18 AM, Reed Kotler <reed.kotler at imgtec.com> >> wrote: >> >>> Well, you are an mclinker contributor >>> >> >> Me personally? Not that I know of. >> >> >> Sorry. I thought i had seen your name in an mclinker commit. >> >> and Google uses mclinker >>> >> >> So I've been told, though I hadn't even heard of mclinker until this >> email thread. >> >> >> It's another linker (we don't have enough of them :) ) . It's used to >> run on mobile devices >> and has been designed with that criteria in mind. (At least that is my >> understanding). >> >> and now it's broken as the result of your change. >>> >> >> This is a pretty standard state of affairs for LLVM-using projects, >> LLVM's APIs change continuously. >> >> >> >> Probably then this is a configuration issue I need to take up with >> mclinker folks. >> Their build instructions have you just download tip of tree from svn and >> that clearly is not guaranteed to work. >> Probably their wiki should list the version from LLVM to download. >> > > I don't know what the deal is with mclinker's strategy here, but some out > of tree projects follow llvm - the nature of that is that it'll break from > time to time (just as in-tree projects break with LLVM API changes (not > many people checkout/build some subprojects like polly, etc)) - for > projects that follow ToT, not any particular snapshot revision, it's just a > matter of who syncs up first & sees the break - everyone usually takes some > responsibility to fix things up if they break. (and/or loop the right > people in if you sync up & it's broken and you don't know how to fix it & > have a chat about how to fix it) > > > I have to confess ignorance here because I'm just following their wiki > instructions which are written as if it's TOT compatible with LLVM. > > >> >> I still don't see any justification to making a change in a public >>> interface that is used by other non LLVM projects >>> to fix some issue with clang warnings. People should be able to derive >>> from those classes. I can't understand >>> your reasoning as to why these classes must be final. >>> >> >> Because if they're not, it's easy to write bugs with them - especially in >> non-tree projects, strangely enough. (if you derive from this type, then >> happen to own those derived objects polymorphically, you'll invoke >> undefined behavior - by making the type 'final' it removes the ability to >> write such bugs). >> >> >> Isn't that a problem with the design of this class? (or these classes)? >> > > Not really - not for LLVM's use cases (which are the ones the API is > designed for, of course). We could make the dtor virtual instead - but > that's unnecessary overhead compared to just ensuring (by use of protected > dtors and final classes) that the class can't be destroyed polymorphically > anyway. It's a legitimate design to just disallow it rather than adding > overhead to allow it when it's not needed by the use cases we have. > > > Is it really necessary to worry about the overhead of virtual destructors > for command line processing classes? > It makes the classes less generally useful for no measurable benefit. >I rather like the notion of not paying for what one doesn't use (a common cliche/principle of C++) - until there's a use case (& granted, use cases don't need to be in-tree, but it'd need to be justified & it's not clear mclinker's use is) I'd err on the side of not paying anything for something not used.> > > >> >> I was kind of surprised that there are no mclinker build bots that >>> would have picked this up right away. >>> >>> >>> On 03/19/2015 09:08 AM, David Blaikie wrote: >>> >>> >>> >>> On Thu, Mar 19, 2015 at 9:05 AM, Reed Kotler <reed.kotler at imgtec.com> >>> wrote: >>> >>>> On 03/19/2015 08:55 AM, David Blaikie wrote: >>>> >>>> >>>> >>>> On Thu, Mar 19, 2015 at 4:30 AM, Reed Kotler <Reed.Kotler at imgtec.com> >>>> wrote: >>>> >>>>> One could argue that mclinker is doing something good or not by how >>>>> it's using this class >>>>> but I don't see the need for parser<bool> to be final. That is a >>>>> subjective opinion that mclinker needs to >>>>> be changed. >>>>> >>>>> I think that "final" was added to some of these command line classes >>>>> to avoid some kind of clang warning. >>>>> >>>>> That seems wrong to me that the tools are dictating something in an >>>>> API. >>>>> >>>> >>>> Well the warning's a pretty good one - and dictates a way to design an >>>> API safely to avoid the possibility of a bad delete. >>>> >>>> It's not uncommon to have a polymorphic hierarchy that doesn't require >>>> polymorphic ownership. To ensure that polymorphic ownership doesn't sneak >>>> in, we have a warning to ensure the class either has an interface >>>> compatible with polymorphic ownership (has a virtual dtor) or disallows >>>> polymorphic ownership (has a protected dtor in any non-final base). >>>> >>>> There is a narrower warning that only diagnoses this at the use of >>>> polymorphic destruction (-Wdelete-non-virtual-dtor) but we've not needed to >>>> fallback to that in LLVM so far & I'd mildly rather avoid doing so. The >>>> protected-final-or-virtual pattern seems fairly elegant/self-documenting to >>>> me. >>>> >>>> >>>>> There may be other valid reasons to create derived classes. >>>>> >>>>> I think we should remove the "final" attribute on those classes and >>>>> fix the clang warning issue in some >>>>> better way than changing the API. >>>>> >>>>> If mclinker had been part of the llvm project tools tree that would >>>>> not have been allowed even. >>>>> >>>> >>>> Or we might've decided that the use in mclinker was bad & should be >>>> fixed - I haven't really looked at it to consider the details of its use, >>>> to be honest. >>>> >>>> >>>> There are lots of valid reasons to want to be able to derive classes; >>>> that's why C++ lets you do that. :) >>>> >>>> I don't think that LLVM judging MCLinker coding is relevant here. >>>> >>> >>> It seems like it is to me - when refactoring code we look at the use >>> cases and judge what the right way to handle those use cases is. One of >>> those ways is to fix/change/remove the use-cases that seem >>> problematic/unnecessary/incorrect. >>> >>> >>>> Lots of projects use LLVM now and changing the API in this way >>>> does not have justification other than to deal with some clang warning >>>> issues that can be solved in other ways. >>>> >>> >>> I think the practices that the clang warning enforces/encourages are >>> good ones, otherwise I would've fixed/changed/disabled the warning. We've >>> used these practices/this warning for a while now & it seems to suit LLVM's >>> use cases pretty well so far. >>> >>> >>>> I don't see any justification for deciding once and for all that there >>>> are no valid reasons for deriving from these classes >>>> and to use a c++ feature to enforce that. >>>> >>>> As I said earlier, had mclinker been in the tools subdirectory of LLVM >>>> then the change the CommandLine.h would not have even been allowed. >>>> >>>> >>>> >>>> >>>> >>>>> ________________________________________ >>>>> From: Simon Atanasyan [simon at atanasyan.com] >>>>> Sent: Wednesday, March 18, 2015 9:35 PM >>>>> To: Reed Kotler >>>>> Cc: David Blaikie; Simon Atanasyan; Rich Fuhler; llvmdev at cs.uiuc.edu >>>>> Subject: Re: [LLVMdev] Final added to parser<bool> >>>>> >>>>> Hi Reed, >>>>> >>>>> The FalseParser has a rather strange design. It's purpose to parse >>>>> options like -no-to-do-something and always return false even if a >>>>> user provides -no-to-do-something=no. That should be fixed on the >>>>> MCLinker side. >>>>> >>>>> On Thu, Mar 19, 2015 at 5:14 AM, reed kotler <rkotler at mips.com> wrote: >>>>> > >>>>> //===----------------------------------------------------------------------===// >>>>> > // FalseParser >>>>> > >>>>> //===----------------------------------------------------------------------===// >>>>> > class FalseParser : public parser<bool> { >>>>> > public: >>>>> > explicit FalseParser(Option &O) : parser<bool>(O) { } >>>>> > >>>>> > // parse - Return true on error. >>>>> > bool parse(cl::Option& O, StringRef ArgName, StringRef Arg, bool& >>>>> Val) { >>>>> > if (cl::parser<bool>::parse(O, ArgName, Arg, Val)) >>>>> > return false; >>>>> > Val = false; >>>>> > return false; >>>>> > } >>>>> > }; >>>>> > >>>>> > I don't know the history of this. I'm just starting to do some >>>>> mclinker work >>>>> > to add the new mips r6 relocations to it. >>>>> > >>>>> > >>>>> > On 03/18/2015 07:00 PM, David Blaikie wrote: >>>>> > >>>>> > >>>>> > >>>>> > On Wed, Mar 18, 2015 at 6:48 PM, reed kotler <rkotler at mips.com> >>>>> wrote: >>>>> >> >>>>> >> Hi David, >>>>> >> >>>>> >> Is there a reason that we need to have "final" for parser<bool> ??? >>>>> > >>>>> > >>>>> > Clang has a (reasonable) warning for types with virtual functions >>>>> and a >>>>> > non-virtual dtor. This warning is suppressed if the dtor is >>>>> protected or the >>>>> > class is final (since in the first case it's clear that the user >>>>> intends not >>>>> > to destroy objects via base pointers, only derived ones - and in the >>>>> second >>>>> > case there's no risk of derived classes, so public access to the >>>>> dtor is >>>>> > safe even without virtual dispatch. >>>>> > >>>>> > Since the parser hierarchy never needed polymorphic destruction (all >>>>> > instances are concrete instances of derived classes owned and >>>>> destroyed >>>>> > directly, not via base pointers) this seemed like a fine way to >>>>> structure >>>>> > the API. >>>>> >>>>> -- >>>>> Simon Atanasyan >>>>> >>>> >>>> >>>> >>> >>> >> >> > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150319/101ba114/attachment.html>
On 03/19/2015 09:57 AM, David Blaikie wrote:> > > On Thu, Mar 19, 2015 at 9:52 AM, Reed Kotler <reed.kotler at imgtec.com > <mailto:reed.kotler at imgtec.com>> wrote: > > On 03/19/2015 09:38 AM, David Blaikie wrote: >> >> >> On Thu, Mar 19, 2015 at 9:34 AM, Reed Kotler >> <reed.kotler at imgtec.com <mailto:reed.kotler at imgtec.com>> wrote: >> >> On 03/19/2015 09:24 AM, David Blaikie wrote: >>> >>> >>> On Thu, Mar 19, 2015 at 9:18 AM, Reed Kotler >>> <reed.kotler at imgtec.com <mailto:reed.kotler at imgtec.com>> wrote: >>> >>> Well, you are an mclinker contributor >>> >>> >>> Me personally? Not that I know of. >> Sorry. I thought i had seen your name in an mclinker commit. >>> >>> and Google uses mclinker >>> >>> >>> So I've been told, though I hadn't even heard of mclinker >>> until this email thread. >> It's another linker (we don't have enough of them :) ) . It's >> used to run on mobile devices >> and has been designed with that criteria in mind. (At least >> that is my understanding). >> >>> and now it's broken as the result of your change. >>> >>> >>> This is a pretty standard state of affairs for LLVM-using >>> projects, LLVM's APIs change continuously. >> Probably then this is a configuration issue I need to take up >> with mclinker folks. >> Their build instructions have you just download tip of tree >> from svn and that clearly is not guaranteed to work. >> Probably their wiki should list the version from LLVM to >> download. >> >> >> I don't know what the deal is with mclinker's strategy here, but >> some out of tree projects follow llvm - the nature of that is >> that it'll break from time to time (just as in-tree projects >> break with LLVM API changes (not many people checkout/build some >> subprojects like polly, etc)) - for projects that follow ToT, not >> any particular snapshot revision, it's just a matter of who syncs >> up first & sees the break - everyone usually takes some >> responsibility to fix things up if they break. (and/or loop the >> right people in if you sync up & it's broken and you don't know >> how to fix it & have a chat about how to fix it) > I have to confess ignorance here because I'm just following their > wiki instructions which are written as if it's TOT compatible with > LLVM. >> >> >> >>> I still don't see any justification to making a change >>> in a public interface that is used by other non LLVM >>> projects >>> to fix some issue with clang warnings. People should be >>> able to derive from those classes. I can't understand >>> your reasoning as to why these classes must be final. >>> >>> >>> Because if they're not, it's easy to write bugs with them - >>> especially in non-tree projects, strangely enough. (if you >>> derive from this type, then happen to own those derived >>> objects polymorphically, you'll invoke undefined behavior - >>> by making the type 'final' it removes the ability to write >>> such bugs). >> Isn't that a problem with the design of this class? (or these >> classes)? >> >> >> Not really - not for LLVM's use cases (which are the ones the API >> is designed for, of course). We could make the dtor virtual >> instead - but that's unnecessary overhead compared to just >> ensuring (by use of protected dtors and final classes) that the >> class can't be destroyed polymorphically anyway. It's a >> legitimate design to just disallow it rather than adding overhead >> to allow it when it's not needed by the use cases we have. > Is it really necessary to worry about the overhead of virtual > destructors for command line processing classes? > It makes the classes less generally useful for no measurable benefit. > > > I rather like the notion of not paying for what one doesn't use (a > common cliche/principle of C++) - until there's a use case (& granted, > use cases don't need to be in-tree, but it'd need to be justified & > it's not clear mclinker's use is) I'd err on the side of not paying > anything for something not used.IMHO, I think it's a common LLVM cliche to worry about non measurable performance issues at the cost of making the code more complex and more difficult to maintain. I don't think it's a common cliche/principle of C++ to not pay for things you don't use except as the language gives you some of that for free (in tow with a good optimizing compiler). I prefer to make things more general if making them more specific adds complexity and/or makes them less useful later. Adding complexity to get increased performance/space savings to me is what needs to be justified. Rather than just have virtual destructors here, there are lots of complex C++ issues at play which in the end could only be solved by another complexity of adding the "final" keyword which now restricts the usefulness of the classes. We have a different point of view here. Reed> > >> >> >>> I was kind of surprised that there are no mclinker build >>> bots that would have picked this up right away. >>> >>> >>> On 03/19/2015 09:08 AM, David Blaikie wrote: >>>> >>>> >>>> On Thu, Mar 19, 2015 at 9:05 AM, Reed Kotler >>>> <reed.kotler at imgtec.com >>>> <mailto:reed.kotler at imgtec.com>> wrote: >>>> >>>> On 03/19/2015 08:55 AM, David Blaikie wrote: >>>>> >>>>> >>>>> On Thu, Mar 19, 2015 at 4:30 AM, Reed Kotler >>>>> <Reed.Kotler at imgtec.com >>>>> <mailto:Reed.Kotler at imgtec.com>> wrote: >>>>> >>>>> One could argue that mclinker is doing >>>>> something good or not by how it's using this class >>>>> but I don't see the need for parser<bool> to >>>>> be final. That is a subjective opinion that >>>>> mclinker needs to >>>>> be changed. >>>>> >>>>> I think that "final" was added to some of >>>>> these command line classes to avoid some kind >>>>> of clang warning. >>>>> >>>>> That seems wrong to me that the tools are >>>>> dictating something in an API. >>>>> >>>>> >>>>> Well the warning's a pretty good one - and >>>>> dictates a way to design an API safely to avoid >>>>> the possibility of a bad delete. >>>>> >>>>> It's not uncommon to have a polymorphic hierarchy >>>>> that doesn't require polymorphic ownership. To >>>>> ensure that polymorphic ownership doesn't sneak >>>>> in, we have a warning to ensure the class either >>>>> has an interface compatible with polymorphic >>>>> ownership (has a virtual dtor) or disallows >>>>> polymorphic ownership (has a protected dtor in any >>>>> non-final base). >>>>> >>>>> There is a narrower warning that only diagnoses >>>>> this at the use of polymorphic destruction >>>>> (-Wdelete-non-virtual-dtor) but we've not needed >>>>> to fallback to that in LLVM so far & I'd mildly >>>>> rather avoid doing so. The >>>>> protected-final-or-virtual pattern seems fairly >>>>> elegant/self-documenting to me. >>>>> >>>>> There may be other valid reasons to create >>>>> derived classes. >>>>> >>>>> I think we should remove the "final" attribute >>>>> on those classes and fix the clang warning >>>>> issue in some >>>>> better way than changing the API. >>>>> >>>>> If mclinker had been part of the llvm project >>>>> tools tree that would not have been allowed even. >>>>> >>>>> >>>>> Or we might've decided that the use in mclinker >>>>> was bad & should be fixed - I haven't really >>>>> looked at it to consider the details of its use, >>>>> to be honest. >>>> There are lots of valid reasons to want to be able >>>> to derive classes; that's why C++ lets you do that. :) >>>> >>>> I don't think that LLVM judging MCLinker coding is >>>> relevant here. >>>> >>>> >>>> It seems like it is to me - when refactoring code we >>>> look at the use cases and judge what the right way to >>>> handle those use cases is. One of those ways is to >>>> fix/change/remove the use-cases that seem >>>> problematic/unnecessary/incorrect. >>>> >>>> Lots of projects use LLVM now and changing the API >>>> in this way >>>> does not have justification other than to deal with >>>> some clang warning issues that can be solved in >>>> other ways. >>>> >>>> >>>> I think the practices that the clang warning >>>> enforces/encourages are good ones, otherwise I would've >>>> fixed/changed/disabled the warning. We've used these >>>> practices/this warning for a while now & it seems to >>>> suit LLVM's use cases pretty well so far. >>>> >>>> I don't see any justification for deciding once and >>>> for all that there are no valid reasons for >>>> deriving from these classes >>>> and to use a c++ feature to enforce that. >>>> >>>> As I said earlier, had mclinker been in the tools >>>> subdirectory of LLVM then the change the >>>> CommandLine.h would not have even been allowed. >>>> >>>> >>>> >>>> >>>>> >>>>> ________________________________________ >>>>> From: Simon Atanasyan [simon at atanasyan.com >>>>> <mailto:simon at atanasyan.com>] >>>>> Sent: Wednesday, March 18, 2015 9:35 PM >>>>> To: Reed Kotler >>>>> Cc: David Blaikie; Simon Atanasyan; Rich >>>>> Fuhler; llvmdev at cs.uiuc.edu >>>>> <mailto:llvmdev at cs.uiuc.edu> >>>>> Subject: Re: [LLVMdev] Final added to parser<bool> >>>>> >>>>> Hi Reed, >>>>> >>>>> The FalseParser has a rather strange design. >>>>> It's purpose to parse >>>>> options like -no-to-do-something and always >>>>> return false even if a >>>>> user provides -no-to-do-something=no. That >>>>> should be fixed on the >>>>> MCLinker side. >>>>> >>>>> On Thu, Mar 19, 2015 at 5:14 AM, reed kotler >>>>> <rkotler at mips.com <mailto:rkotler at mips.com>> >>>>> wrote: >>>>> > >>>>> //===----------------------------------------------------------------------===// >>>>> > // FalseParser >>>>> > >>>>> //===----------------------------------------------------------------------===// >>>>> > class FalseParser : public parser<bool> { >>>>> > public: >>>>> > explicit FalseParser(Option &O) : >>>>> parser<bool>(O) { } >>>>> > >>>>> > // parse - Return true on error. >>>>> > bool parse(cl::Option& O, StringRef >>>>> ArgName, StringRef Arg, bool& Val) { >>>>> > if (cl::parser<bool>::parse(O, ArgName, >>>>> Arg, Val)) >>>>> > return false; >>>>> > Val = false; >>>>> > return false; >>>>> > } >>>>> > }; >>>>> > >>>>> > I don't know the history of this. I'm just >>>>> starting to do some mclinker work >>>>> > to add the new mips r6 relocations to it. >>>>> > >>>>> > >>>>> > On 03/18/2015 07:00 PM, David Blaikie wrote: >>>>> > >>>>> > >>>>> > >>>>> > On Wed, Mar 18, 2015 at 6:48 PM, reed kotler >>>>> <rkotler at mips.com <mailto:rkotler at mips.com>> >>>>> wrote: >>>>> >> >>>>> >> Hi David, >>>>> >> >>>>> >> Is there a reason that we need to have >>>>> "final" for parser<bool> ??? >>>>> > >>>>> > >>>>> > Clang has a (reasonable) warning for types >>>>> with virtual functions and a >>>>> > non-virtual dtor. This warning is suppressed >>>>> if the dtor is protected or the >>>>> > class is final (since in the first case it's >>>>> clear that the user intends not >>>>> > to destroy objects via base pointers, only >>>>> derived ones - and in the second >>>>> > case there's no risk of derived classes, so >>>>> public access to the dtor is >>>>> > safe even without virtual dispatch. >>>>> > >>>>> > Since the parser hierarchy never needed >>>>> polymorphic destruction (all >>>>> > instances are concrete instances of derived >>>>> classes owned and destroyed >>>>> > directly, not via base pointers) this seemed >>>>> like a fine way to structure >>>>> > the API. >>>>> >>>>> -- >>>>> Simon Atanasyan >>>>> >>>>> >>>> >>>> >>> >>> >> >> > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150319/27a80d11/attachment.html>