[PATCH] D148692: Fix uninitialized class members

Fri May 12 10:54:31 PDT 2023

dblaikie added a comment.

In D148692#4338228 <https://reviews.llvm.org/D148692#4338228>, @vitalybuka wrote:

> In D148692#4338165 <https://reviews.llvm.org/D148692#4338165>, @dblaikie wrote:
>
>> Adding initializers to variables that don't need it (because through some dynamic path they are initialized before they're used) may hinder tools like msan from finding real bugs.
>
> It's can be useful for local variables. But for class members, especially complex, when half members are already initialized, it's seems cont-productive.

Actually for class members there are cases where it comes to mind - when a member is only used under some substate of the object, etc. (we do also have compile time warnings to catch some of these cases too, that aren't just "make sure everything's always initialized at the declaration" - checking for private members that are only ever read and never written, which is admittedly a smaller set of cases)

> We will more often introduce bugs then catch some with msan.

Happen to have data on this?

> Unless it's not perf issue, I would rather see consistent initialized state.
> Dynamic checks do not cover all cases in testing, plus optimizations suppress some bugs.
>
> Fopr logical bug I would rather recommend to set default to some invalid value, and then assert() or diagnostics.

Not everything has invalid values.

In any case, I think it'd be a pretty major stylistic requirement for LLVM if we're going to adopt initializing members at declaration in all cases - maybe worth a style discussion on the forums, etc, before we go adding these where they aren't demonstrably needed/only motivated by a stylistic checker.

If we do want to adopt such a policy, it seems like a pretty easy one to enforce/check/etc, rather than these sporadic "someone ran a checker" situations.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148692