<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 19, 2015 at 9:52 AM, Reed Kotler <span dir="ltr"><<a href="mailto:reed.kotler@imgtec.com" target="_blank">reed.kotler@imgtec.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  
    
  
  <div bgcolor="#FFFFFF" text="#000000"><span class="">
    <div>On 03/19/2015 09:38 AM, David Blaikie
      wrote:<br>
    </div>
    <blockquote type="cite">
      
      <div dir="ltr"><br>
        <div class="gmail_extra"><br>
          <div class="gmail_quote">On Thu, Mar 19, 2015 at 9:34 AM, Reed
            Kotler <span dir="ltr"><<a href="mailto:reed.kotler@imgtec.com" target="_blank">reed.kotler@imgtec.com</a>></span>
            wrote:<br>
            <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
              <div bgcolor="#FFFFFF" text="#000000"><span>
                  <div>On 03/19/2015 09:24 AM, David Blaikie wrote:<br>
                  </div>
                  <blockquote type="cite">
                    <div dir="ltr"><br>
                      <div class="gmail_extra"><br>
                        <div class="gmail_quote">On Thu, Mar 19, 2015 at
                          9:18 AM, Reed Kotler <span dir="ltr"><<a href="mailto:reed.kotler@imgtec.com" target="_blank">reed.kotler@imgtec.com</a>></span>
                          wrote:<br>
                          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
                            <div bgcolor="#FFFFFF" text="#000000">
                              <div>Well, you are an mclinker contributor
                              </div>
                            </div>
                          </blockquote>
                          <div><br>
                            Me personally? Not that I know of.<br>
                             </div>
                        </div>
                      </div>
                    </div>
                  </blockquote>
                </span> Sorry. I thought i had seen your name in an
                mclinker commit.<span><br>
                  <blockquote type="cite">
                    <div dir="ltr">
                      <div class="gmail_extra">
                        <div class="gmail_quote">
                          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
                            <div bgcolor="#FFFFFF" text="#000000">
                              <div> and Google uses mclinker</div>
                            </div>
                          </blockquote>
                          <div><br>
                            So I've been told, though I hadn't even
                            heard of mclinker until this email thread.<br>
                             </div>
                        </div>
                      </div>
                    </div>
                  </blockquote>
                </span> It's another linker (we don't have enough of
                them :) ) . It's used to run on mobile devices<br>
                and has been designed with that criteria in mind. (At
                least that is my understanding).<span><br>
                  <br>
                  <blockquote type="cite">
                    <div dir="ltr">
                      <div class="gmail_extra">
                        <div class="gmail_quote">
                          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
                            <div bgcolor="#FFFFFF" text="#000000">
                              <div> and now it's broken as the result of
                                your change.<br>
                              </div>
                            </div>
                          </blockquote>
                          <div><br>
                            This is a pretty standard state of affairs
                            for LLVM-using projects, LLVM's APIs change
                            continuously. <br>
                          </div>
                        </div>
                      </div>
                    </div>
                  </blockquote>
                  <blockquote type="cite">
                    <div dir="ltr">
                      <div class="gmail_extra">
                        <div class="gmail_quote">
                          <div> </div>
                        </div>
                      </div>
                    </div>
                  </blockquote>
                </span> Probably then this is a configuration issue I
                need to take up with mclinker folks.<br>
                Their build instructions have you just download tip of
                tree from svn and that clearly is not guaranteed to
                work.<br>
                Probably their wiki should list the version from LLVM to
                download.</div>
            </blockquote>
            <div><br>
              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)<br>
               </div>
          </div>
        </div>
      </div>
    </blockquote></span>
    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.<span class=""><br>
    <blockquote type="cite">
      <div dir="ltr">
        <div class="gmail_extra">
          <div class="gmail_quote">
            <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
              <div bgcolor="#FFFFFF" text="#000000"><span><br>
                  <br>
                  <blockquote type="cite">
                    <div dir="ltr">
                      <div class="gmail_extra">
                        <div class="gmail_quote">
                          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
                            <div bgcolor="#FFFFFF" text="#000000">
                              <div>I still don't see any justification
                                to making a change in a public interface
                                that is used by other non LLVM projects<br>
                                to fix some issue with clang warnings.
                                People should be able to derive from
                                those classes. I can't understand<br>
                                your reasoning as to why these classes
                                must be final.<br>
                              </div>
                            </div>
                          </blockquote>
                          <div><br>
                            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).<br>
                             </div>
                        </div>
                      </div>
                    </div>
                  </blockquote>
                </span> Isn't that a problem with the design of this
                class? (or these classes)?</div>
            </blockquote>
            <div><br>
              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.<br>
               </div>
          </div>
        </div>
      </div>
    </blockquote></span>
    Is it really necessary to worry about the overhead of virtual
    destructors for command line processing classes?<br>
    It makes the classes less generally useful for no measurable
    benefit.</div></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div><div class="h5"><br>
    <br>
    <blockquote type="cite">
      <div dir="ltr">
        <div class="gmail_extra">
          <div class="gmail_quote">
            <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
              <div bgcolor="#FFFFFF" text="#000000">
                <div>
                  <div><br>
                    <br>
                    <blockquote type="cite">
                      <div dir="ltr">
                        <div class="gmail_extra">
                          <div class="gmail_quote">
                            <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
                              <div bgcolor="#FFFFFF" text="#000000">
                                <div>I was kind of surprised that there
                                  are no mclinker build bots that would
                                  have picked this up right away.
                                  <div>
                                    <div><br>
                                      <br>
                                      On 03/19/2015 09:08 AM, David
                                      Blaikie wrote:<br>
                                    </div>
                                  </div>
                                </div>
                                <div>
                                  <div>
                                    <blockquote type="cite">
                                      <div dir="ltr"><br>
                                        <div class="gmail_extra"><br>
                                          <div class="gmail_quote">On
                                            Thu, Mar 19, 2015 at 9:05
                                            AM, Reed Kotler <span dir="ltr"><<a href="mailto:reed.kotler@imgtec.com" target="_blank">reed.kotler@imgtec.com</a>></span>
                                            wrote:<br>
                                            <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
                                              <div bgcolor="#FFFFFF" text="#000000"><span>
                                                  <div>On 03/19/2015
                                                    08:55 AM, David
                                                    Blaikie wrote:<br>
                                                  </div>
                                                  <blockquote type="cite">
                                                    <div dir="ltr"><br>
                                                      <div class="gmail_extra"><br>
                                                        <div class="gmail_quote">On
                                                          Thu, Mar 19,
                                                          2015 at 4:30
                                                          AM, Reed
                                                          Kotler <span dir="ltr"><<a href="mailto:Reed.Kotler@imgtec.com" target="_blank">Reed.Kotler@imgtec.com</a>></span>
                                                          wrote:<br>
                                                          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">One

                                                          could argue
                                                          that mclinker
                                                          is doing
                                                          something good
                                                          or not by how
                                                          it's using
                                                          this class<br>
                                                          but I don't
                                                          see the need
                                                          for
                                                          parser<bool>
                                                          to be final.
                                                          That is a
                                                          subjective
                                                          opinion that
                                                          mclinker needs
                                                          to<br>
                                                          be changed.<br>
                                                          <br>
                                                          I think that
                                                          "final" was
                                                          added  to some
                                                          of these
                                                          command line
                                                          classes to
                                                          avoid some
                                                          kind of clang
                                                          warning.<br>
                                                          <br>
                                                          That seems
                                                          wrong to me
                                                          that the tools
                                                          are dictating
                                                          something in
                                                          an API.<br>
                                                          </blockquote>
                                                          <div><br>
                                                          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.<br>
                                                          <br>
                                                          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).<br>
                                                          <br>
                                                          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.<br>
                                                           </div>
                                                          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">There

                                                          may be other
                                                          valid reasons
                                                          to create
                                                          derived
                                                          classes.<br>
                                                          <br>
                                                          I think we
                                                          should remove
                                                          the "final"
                                                          attribute on
                                                          those classes
                                                          and fix the
                                                          clang warning
                                                          issue in some<br>
                                                          better way
                                                          than changing
                                                          the API.<br>
                                                          <br>
                                                          If mclinker
                                                          had been part
                                                          of the llvm
                                                          project tools
                                                          tree that
                                                          would not have
                                                          been allowed
                                                          even.<br>
                                                          </blockquote>
                                                          <div><br>
                                                          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.<br>
                                                           </div>
                                                        </div>
                                                      </div>
                                                    </div>
                                                  </blockquote>
                                                </span> There are lots
                                                of valid reasons to want
                                                to be able to derive
                                                classes; that's why C++
                                                lets you do that. :)<br>
                                                <br>
                                                I don't think that LLVM
                                                judging MCLinker coding
                                                is relevant here. </div>
                                            </blockquote>
                                            <div><br>
                                              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.<br>
                                               </div>
                                            <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
                                              <div bgcolor="#FFFFFF" text="#000000"> Lots of
                                                projects use LLVM now
                                                and changing the API in
                                                this way<br>
                                                does not have
                                                justification other than
                                                to deal with some clang
                                                warning issues that can
                                                be solved in other ways.<br>
                                              </div>
                                            </blockquote>
                                            <div><br>
                                              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.<br>
                                               </div>
                                            <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
                                              <div bgcolor="#FFFFFF" text="#000000">I don't
                                                see any justification
                                                for deciding once and
                                                for all that there are
                                                no valid reasons for
                                                deriving from these
                                                classes<br>
                                                and to use a c++ feature
                                                to enforce that.<br>
                                                <br>
                                                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.
                                                <div>
                                                  <div><br>
                                                    <br>
                                                    <br>
                                                    <br>
                                                    <blockquote type="cite">
                                                      <div dir="ltr">
                                                        <div class="gmail_extra">
                                                          <div class="gmail_quote">
                                                          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
                                                          <br>
________________________________________<br>
                                                          From: Simon
                                                          Atanasyan [<a href="mailto:simon@atanasyan.com" target="_blank">simon@atanasyan.com</a>]<br>
                                                          Sent:
                                                          Wednesday,
                                                          March 18, 2015
                                                          9:35 PM<br>
                                                          To: Reed
                                                          Kotler<br>
                                                          Cc: David
                                                          Blaikie; Simon
                                                          Atanasyan;
                                                          Rich Fuhler; <a href="mailto:llvmdev@cs.uiuc.edu" target="_blank">llvmdev@cs.uiuc.edu</a><br>
                                                          Subject: Re:
                                                          [LLVMdev]
                                                          Final added to
parser<bool><br>
                                                          <div>
                                                          <div><br>
                                                          Hi Reed,<br>
                                                          <br>
                                                          The
                                                          FalseParser
                                                          has a rather
                                                          strange
                                                          design. It's
                                                          purpose to
                                                          parse<br>
                                                          options like
                                                          -no-to-do-something
                                                          and always
                                                          return false
                                                          even if a<br>
                                                          user provides
                                                          -no-to-do-something=no.

                                                          That should be
                                                          fixed on the<br>
                                                          MCLinker side.<br>
                                                          <br>
                                                          On Thu, Mar
                                                          19, 2015 at
                                                          5:14 AM, reed
                                                          kotler <<a href="mailto:rkotler@mips.com" target="_blank">rkotler@mips.com</a>>

                                                          wrote:<br>
                                                          >
//===----------------------------------------------------------------------===//<br>
                                                          > //
                                                          FalseParser<br>
                                                          >
//===----------------------------------------------------------------------===//<br>
                                                          > class
                                                          FalseParser :
                                                          public
                                                          parser<bool>
                                                          {<br>
                                                          >  public:<br>
                                                          > 
                                                           explicit
                                                          FalseParser(Option
                                                          &O) :
                                                          parser<bool>(O)
                                                          { }<br>
                                                          ><br>
                                                          >   //
                                                          parse - Return
                                                          true on error.<br>
                                                          >   bool
                                                          parse(cl::Option&
                                                          O, StringRef
                                                          ArgName,
                                                          StringRef Arg,
                                                          bool& Val)
                                                          {<br>
                                                          >     if
                                                          (cl::parser<bool>::parse(O,
                                                          ArgName, Arg,
                                                          Val))<br>
                                                          >     
                                                           return false;<br>
                                                          >     Val =
                                                          false;<br>
                                                          >   
                                                           return false;<br>
                                                          >   }<br>
                                                          > };<br>
                                                          ><br>
                                                          > I don't
                                                          know the
                                                          history of
                                                          this. I'm just
                                                          starting to do
                                                          some mclinker
                                                          work<br>
                                                          > to add
                                                          the new mips
                                                          r6 relocations
                                                          to it.<br>
                                                          ><br>
                                                          ><br>
                                                          > On
                                                          03/18/2015
                                                          07:00 PM,
                                                          David Blaikie
                                                          wrote:<br>
                                                          ><br>
                                                          ><br>
                                                          ><br>
                                                          > On Wed,
                                                          Mar 18, 2015
                                                          at 6:48 PM,
                                                          reed kotler
                                                          <<a href="mailto:rkotler@mips.com" target="_blank">rkotler@mips.com</a>>
                                                          wrote:<br>
                                                          >><br>
                                                          >> Hi
                                                          David,<br>
                                                          >><br>
                                                          >> Is
                                                          there a reason
                                                          that we need
                                                          to have
                                                          "final" for
                                                          parser<bool>
                                                          ???<br>
                                                          ><br>
                                                          ><br>
                                                          > Clang has
                                                          a (reasonable)
                                                          warning for
                                                          types with
                                                          virtual
                                                          functions and
                                                          a<br>
                                                          >
                                                          non-virtual
                                                          dtor. This
                                                          warning is
                                                          suppressed if
                                                          the dtor is
                                                          protected or
                                                          the<br>
                                                          > class is
                                                          final (since
                                                          in the first
                                                          case it's
                                                          clear that the
                                                          user intends
                                                          not<br>
                                                          > to
                                                          destroy
                                                          objects via
                                                          base pointers,
                                                          only derived
                                                          ones - and in
                                                          the second<br>
                                                          > case
                                                          there's no
                                                          risk of
                                                          derived
                                                          classes, so
                                                          public access
                                                          to the dtor is<br>
                                                          > safe even
                                                          without
                                                          virtual
                                                          dispatch.<br>
                                                          ><br>
                                                          > Since the
                                                          parser
                                                          hierarchy
                                                          never needed
                                                          polymorphic
                                                          destruction
                                                          (all<br>
                                                          > instances
                                                          are concrete
                                                          instances of
                                                          derived
                                                          classes owned
                                                          and destroyed<br>
                                                          > directly,
                                                          not via base
                                                          pointers) this
                                                          seemed like a
                                                          fine way to
                                                          structure<br>
                                                          > the API.<br>
                                                          <br>
                                                          --<br>
                                                          Simon
                                                          Atanasyan<br>
                                                          </div>
                                                          </div>
                                                          </blockquote>
                                                          </div>
                                                          <br>
                                                        </div>
                                                      </div>
                                                    </blockquote>
                                                    <br>
                                                  </div>
                                                </div>
                                              </div>
                                            </blockquote>
                                          </div>
                                          <br>
                                        </div>
                                      </div>
                                    </blockquote>
                                    <br>
                                  </div>
                                </div>
                              </div>
                            </blockquote>
                          </div>
                          <br>
                        </div>
                      </div>
                    </blockquote>
                    <br>
                  </div>
                </div>
              </div>
            </blockquote>
          </div>
          <br>
        </div>
      </div>
    </blockquote>
    <br>
  </div></div></div>

</blockquote></div><br></div></div>