//===----------------------------------------------------------------------===//
// 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.
>
> This breaks the compilation of mclinker which derives a class from
> this.
>
> In file included from
> /home/rkotler/workspace/mclinker/lib/Support/CommandLine.cpp:9:0:
>
/home/rkotler/workspace/mclinker/include/mcld/Support/CommandLine.h:49:7:
> error: cannot derive from ‘final’ base ‘llvm::cl::parser<bool>’
in
> derived type ‘llvm::cl::FalseParser’
>
>
> Why is it being derived from? If it's just a typedef that's
required,
> it might be more appropriate to use a typedef instead of derivation.
>
> make[2]: *** [Support/CommandLine.o] Error 1
> make[2]: Leaving directory `/home/rkotler/mclinker_build/lib'
> make[1]: *** [all] Error 2
> make[1]: Leaving directory `/home/rkotler/mclinker_build/lib'
> make: *** [all-recursive] Error 1
>
>
>
> Reed
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150318/1bd9bec1/attachment.html>
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
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. 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. ________________________________________ 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