NoQ accepted this revision.
NoQ added a comment.
Ok, let's land this one and see how it goes! I'm looking forward to seeing the
follow-up patches.
================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:359-372
+ // Checking bases.
+ const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD);
+ if (!CXXRD)
+ return ContainsUninitField;
+
+ for (const CXXBaseSpecifier BaseSpec : CXXRD->bases()) {
+ const auto *Base = BaseSpec.getType()->getAsCXXRecordDecl();
----------------
Szelethus wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Szelethus wrote:
> > > > NoQ wrote:
> > > > > Are there many warnings that will be caused by this but won't be
> > > > > caused by conducting the check for the constructor-within-constructor
> > > > > that's currently disabled? Not sure - is it even syntactically
> > > > > possible to not initialize the base class?
> > > > I'm not a 100% sure what you mean. Can you clarify?
> > > > >Not sure - is it even syntactically possible to not initialize the
> > > > >base class?
> > > > If I understand the question correctly, no, as far as I know.
> > > You have a check for `isCalledByConstructor()` in order to skip base
> > > class constructors but then you check them manually. That's a lot of
> > > code, but it seems that the same result could have been achieved by
> > > simply skipping the descent into the base class.
> > >
> > > Same question for class-type fields, actually.
> > >Same question for class-type fields, actually.
> >
> > My problem with that approach is that the same uninitialized field could've
> > been emitted multiple times in different warnings. From an earlier
> > conversation (assume that its run in pedantic mode):
> >
> > >For this code snippet:
> > >
> > > ```
> > >struct A {
> > > struct B {
> > > int x, y;
> > >
> > > B() {}
> > > } b;
> > > int w;
> > >
> > > A() {
> > > b = B();
> > > }
> > >};
> > >```
> > >the warning message after a call to `A::A()` would be "3 uninitialized
> > >fields[...]", and for `B::B()` inside `A`s constructor would be "2
> > >uninitialized fields[...]", so it wouldn't be filtered out.
> >
> > In my opinion, if `A` contains a field of type `B`, it should be the
> > responsibility of `A`'s constructors to initialize those fields properly.
> > >You have a check for isCalledByConstructor() in order to skip base class
> > >constructors but then you check them manually. That's a lot of code, but
> > >it seems that the same result could have been achieved by simply skipping
> > >the descent into the base class.
> > I also believe that a constructor "should be held responsible" for the
> > initialization of the inherited data members. However, in the case of base
> > classes, uniqueing isn't an issue, as in this case:
> > ```
> > struct B {
> > int a;
> > B() {}
> > };
> >
> > struct D : public B {
> > int b;
> > D() {}
> > };
> > ```
> > a call to `D::D()` would not check the inherited member (`a`), if I didn't
> > implement it explicitly. That also means that if `isCalledByConstructor`
> > was refactored to not filter out base classes, we would get two warning
> > each with a single note, reporting `b` and `a` respectively. (I haven't
> > actually checked it, but its a reasonable assumption)
> >
> > Actually, I'm not 100% sure which approach is better: a warning for each
> > base, or one warning for the object, bases included. I personally think one
> > warning per object results in the best user experience.
> >
> > I like both ideas, but I think if we'd come to the conclusion that a
> > warning per base should be the way to go, it should definitely be
> > implemented in a followup patch, as `isCalledByConstructor` is already in
> > line for some major refactoring.
> >
> > What do you think? :)
> >[...]but it seems that the same result could have been achieved by simply
> >skipping the descent into the base class.
> I can't edit inline comments sadly, but I'd just like to point out that it
> wouldn't achieve the same thing, as highlighted above. But, both solution
> would be okay.
I still don't fully understand the reasoning behind who's responsible for
initializing the field, i.e., the exact rule that you're enforcing seems to
cause warnings to depend heavily on meta-information such as where does the
analysis start, which is confusing. Because we're trying to enforce a coding
guideline here, i believe that the guideline itself should be transparent and
easy to explain. In particular, it should be obvious to the user which
constructor is responsible for initializing a field, and by obvious i mean it
should be obvious both from the guideline itself and from the analyzer's
warning, and both of them should be the same "obvious".
https://reviews.llvm.org/D45532
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
