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: <lists.llvm.org/pipermail/llvm-dev/attachments/20150319/27a80d11/attachment.html>
On Thu, Mar 19, 2015 at 10:07 AM, Reed Kotler <reed.kotler at imgtec.com> wrote:> 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> > 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. > > > > 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. >This doesn't seem to be particularly complicated to me & is an idiom seen in multiple places in the LLVM codebase, and reinforced by a clang warning, so it's certainly one I'd expect LLVM developers to have a passing familiarity with, or the ability to get familiar with it fairly easily.> We have a different point of view here. >So we do & that's OK. - David> > 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> >>>> 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: <lists.llvm.org/pipermail/llvm-dev/attachments/20150319/81053ac6/attachment.html>
On 03/19/2015 10:12 AM, David Blaikie wrote:> > > On Thu, Mar 19, 2015 at 10:07 AM, Reed Kotler <reed.kotler at imgtec.com > <mailto:reed.kotler at imgtec.com>> wrote: > > 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. > > > This doesn't seem to be particularly complicated to me & is an idiom > seen in multiple places in the LLVM codebase, and reinforced by a > clang warning, so it's certainly one I'd expect LLVM developers to > have a passing familiarity with, or the ability to get familiar with > it fairly easily. > > We have a different point of view here. > > > So we do & that's OK.Of course. Otherwise there could not be horse races. :)> > - David > > > 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: <lists.llvm.org/pipermail/llvm-dev/attachments/20150319/e39e4db6/attachment.html>