<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    Yup, Static Analyzer has a checker for this, and its current status
    is "opt-in" (i.e., the checker is `optin.cplusplus.VirtualCall`). So
    we it's stable and more or less full-featured and we encourage you
    to try it out, but it's off by default because it finds non-bugs as
    well (like the case B). We should definitely enable-by-default the
    part of it that finds *pure* virtual calls. I wonder why didn't we
    do that already. There's even an option to enable such behavior.<br>
    <br>
    Yes, indeed, cases like "C" are the reason why this checker was
    made.<br>
    <br>
    And that's also the reason why this check requires something as
    powerful as a full-featured "symbolic execution" to be useful, which
    is something that's too slow to be a compiler warning. The previous
    attempt on this checker was simply scanning the AST for virtual
    function calls and went through the call graph to see if some of
    these virtual calls are reachable from the constructor. However,
    this approach was having false positives when there was no actual
    execution path that would result in going through the call graph in
    that order during construction. Eg.,<br>
    <br>
        struct D {<br>
            bool Flag;<br>
            void setFlag() { Flag = true; } // The flag can be set
    later.<br>
    <br>
            D() : Flag(false) { foo(); } // But its initial value is
    "clear".<br>
            void foo() { if (Flag) bar(); } // Flag is still clear when
    called from D().<br>
            virtual void bar() = 0;<br>
        }<br>
    <br>
    In this case if you look at the AST you'll see that D() calls foo(),
    foo() calls bar(), bar() is pure virtual. But there's no bug in this
    code, because foo() never actually calls bar() when called from the
    constructor. The VirtualCall checker, in its current state, is able
    to understand this sort of stuff as well (up to overall modeling
    bugs in the Static Analyzer).<br>
    <br>
    A warning could still be used to catch some easier patterns, eg.,
    when *all* paths through the constructor from a certain point result
    in a pure virtual function call. Eg., if you simplify the problem
    down to finding the bug when D::foo() is defined as something like
    `if (Flag) bar(); else bar();`, it may be possible to come up with
    an efficient data flow analysis that will catch it and have no false
    positives. But it still needs to be inter-procedural, so it's
    actually still pretty hard and we will still probably have to bail
    out at some stack depth. This would most likely become the most
    sophisticated compiler warning we have in Clang: we have a few data
    flow warnings, such as "variable may be used uninitialized in this
    function", but none of them are inter-procedural.<br>
    <br>
    So, yeah, i believe that coming up with a compiler warning is indeed
    relatively hard () and implementing it as a Static Analyzer warning
    is most likely the right approach.<br>
    <br>
    <br>
    <div class="moz-cite-prefix">On 12/13/18 12:24 PM, Arthur O'Dwyer
      via cfe-dev wrote:<br>
    </div>
    <blockquote type="cite"
cite="mid:CADvuK0KjzZCS7Bh=7kgcWRK5rQ_CiTq4Gj4MZdrPm0g4BRHjUw@mail.gmail.com">
      <meta http-equiv="content-type" content="text/html; charset=UTF-8">
      <div dir="ltr">
        <div dir="ltr">
          <div dir="ltr">On Thu, Dec 13, 2018 at 1:21 PM Arnaud Bienner
            via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org"
              moz-do-not-send="true">cfe-dev@lists.llvm.org</a>>
            wrote:<br>
            <div class="gmail_quote">
              <blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
                <div dir="ltr">
                  <div>What I like about having this kind of diagnostics
                    being part of clang itself is that you can turn them
                    into errors using -Werror=, and prevent bugs to even
                    been written, saving you debugging time.</div>
                  <div>I'm not sure how static code analysis is done on
                    the projects you are working on, but from my
                    experience, it is usually done as part of the CI (so
                    after you commit your changes): this saves you from
                    having bugs reaching the release stage, but doesn't
                    save you the time spent debugging a silly error that
                    could have been catch by the compiler.</div>
                </div>
              </blockquote>
              <div><br>
              </div>
              <div>FWIW, I think most projects run CI on pull requests,
                so, the code has to pass CI before it's allowed to hit
                master. (This is how you keep your master CI green all
                the time, instead of flashing red-green-red-green.)</div>
              <div><br>
              </div>
              <div>I think any diagnostic in this area is really too
                fragile to be useful. Consider:</div>
              <div><br>
              </div>
              <div>    struct A {</div>
              <div>        A() { do_foo(); }  // diagnosable and
                definitely a bug</div>
              <div>        virtual void do_foo() = 0;<br>
              </div>
              <div>    };</div>
              <div>
                <div>    struct B {</div>
                <div>        B() { do_foo(); }  // diagnosable, but
                  possibly correct</div>
                <div>        virtual void do_foo();</div>
                <div>    };</div>
              </div>
              <div>    struct C {</div>
              <div>        C() { foo(); }  // definitely a bug, but not
                diagnosable</div>
              <div>        void foo() { do_foo(); }  // best practice</div>
              <div>    private:</div>
              <div>        virtual void do_foo() = 0;</div>
              <div>    };</div>
              <div><br>
              </div>
              <div>Clang, GCC, and ICC all diagnose "A". Nobody
                diagnoses "B". Nobody diagnoses "C", even though it's
                exactly isomorphic to "A" — and "C" (splitting
                customization points into a public non-virtual interface
                and a private virtual implementation) is what we want to
                teach people to do!</div>
              <div><br>
              </div>
              <div>I think Clang can't <i>remove</i> its diagnostic for
                "A" because that would be a regression versus GCC and
                ICC; but I don't think it's worth spending time teaching
                Clang to catch "B" (with its attendant false positives)
                given that "C" is the most interesting case in practice,
                and Clang will never be able to catch "C".</div>
              <div><br>
              </div>
              <div>The static analyzer, though, could definitely catch
                "C"! I don't know if it does today or not.</div>
              <div><br>
              </div>
              <div>my $.02,</div>
              <div>–Arthur</div>
            </div>
          </div>
        </div>
      </div>
      <br>
      <fieldset class="mimeAttachmentHeader"></fieldset>
      <pre class="moz-quote-pre" wrap="">_______________________________________________
cfe-dev mailing list
<a class="moz-txt-link-abbreviated" href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a>
</pre>
    </blockquote>
    <br>
  </body>
</html>