[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D103750#2827566 <https://reviews.llvm.org/D103750#2827566>, @RedDocMD wrote:

> Do you want the new failing test to be marked //expected to fail//?

The line of thinking here is that tests are just something that gives us a signal when the behavior changes. They don't necessarily demonstrate the expected behavior. If they don't there's still value in knowing that the observed behavior on them has changed.

For example, if you believe that your commit shouldn't change the observed behavior ("NFC commit", eg. refactoring), any signal that the observed behavior has changed should raise concerns and probably cause you to rethink the commit, even if the behavior has in fact improved.

Similarly, if the functional change is intended, you may still find it suspicious if it suddenly fixes a test that it wasn't supposed to fix. This may help you discover a bug in your commit that would cause the behavior to regress in other related cases.

You don't want the buggy test to be silenced, you want it to keep actively verifying that the bug is not yet fixed.

Then, separately, for every test there's a requirement to keep it understandable, so that the reader would understand what exactly is tested and what are the consequences of breaking the test. It's often valuable to provide comments indicating your level of confidence - "this test is extremely important to pass", "we don't really care about our behavior on this test but it's still nice to know when that behavior changes", etc. - and "we definitely don't want this test to pass but for now it does" is a possible answer as well and it doesn't make the test any less valuable.

So basically

  void foo() {
    // FIXME: Should be FALSE.
    clang_analyzer_eval(1 + 1 == 3); // expected-warning{{TRUE}}
    // FIXME: Should be TRUE.
    clang_analyzer_eval(1 + 1 == 2); // expected-warning{{FALSE}}
  }

is a great test to have.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750