2

I have a piece of C++20 code that I believe is valid, but our static analyzer thinks it's unsafe.

struct Foo {
  explicit Foo() { activeFoo = this; }
  ~Foo() { activeFoo = nullptr; }

  Foo(const Foo&) = delete;
  Foo(Foo&&) = delete;

  inline static const Foo* activeFoo = nullptr;
};

Foo makeFoo()
{
  // Is there a dangling reference here?
  return Foo();
}

int main()
{
  auto myFoo = makeFoo();
}

My static analyzer thinks makeFoo causes activeFoo to point to a temporary object and will become a dangling pointer. I believe this is wrong; the return Foo(); should get a guaranteed copy elision (as evidenced by the deleted copy & move constructors), so there is only ever one Foo instance constructed, myFoo.

Who is correct?

Zizheng Tai
  • 6,170
  • 28
  • 79
  • Which static analyzer? Is it a problem if there are 2 or more Foo objects at the same time? – Eljay Apr 29 '22 at 20:39
  • @Eljay Sonar. In the actual code we throw an exception in the constructor if `activeFoo` is not null, so there can only be one instance at any time. – Zizheng Tai Apr 29 '22 at 20:40
  • Does the static analyzer know the changes to copy elision made in c++17? – user4581301 Apr 29 '22 at 20:51
  • Looks fine to me. Interesting idea that could be of some use instrumenting different code sections. – doug Apr 29 '22 at 21:00
  • There _shouldn't_ be a problem here, but does your actual code delete the copy and move operations too? What about the assignment operators? If so, i'd just iaccept it as a false positive. – Taekahn Apr 29 '22 at 21:13
  • The analyser is right potentially. https://stackoverflow.com/q/48879226/817643 – StoryTeller - Unslander Monica Apr 29 '22 at 21:15
  • @StoryTeller-UnslanderMonica But the destructor is not trivial here, so that shouldn't apply. – user17732522 Apr 29 '22 at 21:18
  • _"and has at least one non-deleted copy or move constructor"_ The move and copy constructors here are all deleted, so that can't apply – Taekahn Apr 29 '22 at 21:19
  • @user17732522 - Assuming the OP did not minimize too much, which often *does* happen. If this is a crtp utility for instance, the actual class using it may be trivial. – StoryTeller - Unslander Monica Apr 29 '22 at 21:20
  • Static analyzers aren't perfect predictors of actual or Standard-specified behavior. And sometimes their false positives are still useful as they point out code which doesn't use best practices or is particularly fragile. – aschepler Apr 29 '22 at 21:43

1 Answers1

4

makeFoo and copy elision (which as you noted is guaranteed in this specific example since C++17) don't even matter.

activeFoo can never be dangling. If it was pointing to an object after its lifetime ended, then the destructor of that object would have reset activeFoo to nullptr, meaning it cannot be pointing to the object, a contradiction. That is assuming the destructor is called for every created object of type Foo. Technically this might not always be the case if you placement-new objects explicitly, although it should.

I would however not generally expect a static analyzer to figure out this logic. Without some details on the static analyzer or what exactly it complains about, it will be hard to say more.

user17732522
  • 53,019
  • 2
  • 56
  • 105
  • 1
    I believe failing to call the destructor of a placement-newed object before it's storage is lost is UB if it has a side effect that can be depended upon. So it shouldn't count as an exception to the first part of your answer. In older versions it was always UB not to call the destructor for class types. – François Andrieux Apr 30 '22 at 00:34
  • 1
    @FrançoisAndrieux Yes, but I don't think it is clear what exactly that restriction means. See https://github.com/cplusplus/draft/pull/2342 and https://cplusplus.github.io/CWG/issues/2523.html. Depending on your interpretation it would be fine to not call the destructor of a `Foo` and reuse its memory for a different type of object, in which case `activeFoo` would be pointing to an object out of lifetime and it would be fine as long as the object isn't accessed through that pointer. In a different interpretation that memory reuse would be UB. – user17732522 Apr 30 '22 at 01:33