Community
Participate
Working Groups
Code examples. The simplest case: struct C { C() {} int v; // <-- Variable "v" is not initialized in constructor. It may cause to random behavior in functions that will read it. } More complex cases: struct C { C() : v1 (0) { v2 = 0; } int v1; // OK: Variable may be initialized in initialization list int v2; // OK: Variable may be initialized in constructor body int v3; // Warning: v3 is not initialized! std::string s; // OK: Variables of complex types (classes & structs) may be no initialized } Of course more complicated cases are possible (e.g. initialization of variable by pointer or reference in another member function that is called from constructor) but it seems complex example above covers the most common used cases. Note: The similar checker exists in cppcheck tool (http://sourceforge.net/projects/cppcheck/).
Created attachment 191487 [details] Basic implementation of the checker described above I have implemented checker for the problem described above. It seems it works fine. Can somebody review and commit or comment it? P.S. Should I set review-flag in cases like this?
(In reply to comment #1) > Created attachment 191487 [details] > Basic implementation of the checker described above > I have implemented checker for the problem described above. It seems it works > fine. Can somebody review and commit or comment it? > P.S. Should I set review-flag in cases like this? Note, in c++ a field will always be initialized. In the example 'v3' will have the value 0.
(In reply to comment #2) > Note, in c++ a field will always be initialized. In the example 'v3' will have > the value 0. You are not right. I have just tried to instantiate this class like this: int main() { C c; cout << c.v3 << endl; return 0; } The output was: -1079585528 And it differs between launches. Maybe your compiler initializes values for you and maybe it is in debug only? I use gcc 4 on Linux, but it seems C++ standard says that class member initial values are not defined. NOTE: In C++ fields are always initialized with 0 if class was created as global or statical.
(In reply to comment #3) You are right, thanks for the clarification. The standard says that members like 'v3' are default initialized. That sounds promising, however for an integral type, default initialization does not do anything.
Created attachment 191496 [details] Basic implementation of the checker described above (with comment fixed) Sorry, I have just notices that I didn't change the comment at the beginning of ClassMembersInitializationCheckerTest file. This patch differs from previous only in that.
I think we should warn for initializer inversions in the same checker. class Foo { int a; int b; public: Foo(): b(0), a(0) // warning {} };
(In reply to comment #6) > I think we should warn for initializer inversions in the same checker. > > class Foo > { > int a; > int b; > public: > Foo(): > b(0), > a(0) // warning > {} > }; I think it should be another checker because: - slightly differ information should be gathered (members declaration order instead of simple declaration). - slightly another construct set should be checked (only initialization list but not assignments). - user may want to turn off or limit scope of violations from one of this checker but leave from another. So I will try to implement it too, but as the separate checker, don't you mind?
Created attachment 191603 [details] Checker implementation with members order check I have looked at Codan preferences once again and noticed that user can turn on/off problems but not checkers. So I agree with you, I should be the same checker. Here its implementation.
(In reply to comment #8) > Created attachment 191603 [details] > Checker implementation with members order check > > I have looked at Codan preferences once again and noticed that user can turn > on/off problems but not checkers. So I agree with you, I should be the same > checker. Here its implementation. There is a few problems with this patch: - it does not take into account order of base classes in initialization list (I have fixed this problem in my working copy). - members order returned by ICPPClassType.getDeclaredFields() is not in which they were declared by user. I don't know how to resolve this problem. Can somebody advise me something? P.S. So the most working patch for now is the second.
Hi, I've been using the checker for a while (second patch), I noticed a false positive with static members: class Foo { Foo(){} // warning here static int bar; }; int Foo::bar = 0;
(In reply to comment #10) > Hi, I've been using the checker for a while (second patch), I noticed a false > positive with static members: > > class Foo > { > Foo(){} // warning here > static int bar; > }; > > int Foo::bar = 0; Oh, yes, you are right. I don't process static member separately. I'll fix this and create a new patch soon. P.S. I have to revert members order check (from the third patch) cause it is not properly implemented and I have not ideas how to fix the problem.
Created attachment 192065 [details] Checker implementation with static members fix (but without member order check) Static members are taken into account now. NOTE: There is not warning in cases like this: class Foo { Foo(){} // warning here static int bar; }; int Foo::bar; // No initialization here Cause static variables are always initialized with 0.
Will somebody review and commit the latest patch? Or is it not useful enough?
(In reply to comment #13) > Will somebody review and commit the latest patch? Or is it not useful enough? It is useful, I've been using it :) I'll have some time in the next days to look at the patch.
(In reply to comment #14) > (In reply to comment #13) > > Will somebody review and commit the latest patch? Or is it not useful enough? > > It is useful, I've been using it :) I'll have some time in the next days to > look at the patch. Thanks, it will be very kind of you! I want this checker to be part of Codan.
Hi, here's what I have so far: - I'm not sure we need getOrResolveBinding, isn't just calling resolveBinding equivalent? - I think all for loops can be replaced by foreach style of loops - Consider 2 classes A and B. If B subclasses A and the analyzed B constructor doesn't call one of the class A constructors and doesn't initialize one of the class A members, then I think it should warn about A members. I think it's reasonable to ignore the the case B constructor calls A constructor but the A constructor doesn't initialize all members. Doing this would be too expensive since we would need to possibly build an additional AST to analyse the A constructor. - testBaseClassMEmberShouldNotBeTakenIntoAccount, there's an extra capital E there. - constructorMap, should be constructorSet instead? Or something more descriptive? fieldsInConstructor? - I think this should be considered a false positive: void func(int & a) { a = 0; } class A { int foo; public: A() // warning { func(foo); } }; - I think this should be considered a false positive: class A { int foo; public: A() // warning { (foo) = 0; } }; - I think this should be considered a false positive. But we may just consider foo assigned as soon as a variable holds a reference to it, just to keep things sane. class A { int foo; public: A() // warning { int & bar = foo; bar = 0; } };
Hi, thank you for review. I have a few questions about cases you (In reply to comment #16) > Hi, here's what I have so far: > > - I'm not sure we need getOrResolveBinding, isn't just calling resolveBinding > equivalent? It seems you are right. I saw this in other checkers and copied to my. I'll fix it. > - Consider 2 classes A and B. If B subclasses A and the analyzed B constructor > doesn't call one of the class A constructors and doesn't initialize one of the > class A members, then I think it should warn about A members. I think it's > reasonable to ignore the the case B constructor calls A constructor but the A > constructor doesn't initialize all members. Doing this would be too expensive > since we would need to possibly build an additional AST to analyse the A > constructor. If B constructor does not call A constructor directly the default A constructor will be called. So warnings will be added to the following correct case: struct A { A() : i(0) {} int a; }; struct B { B() {} // A() will be called by default => A::a will be initialized }; > - I think this should be considered a false positive: > void func(int & a) > { > a = 0; > } > > class A { > int foo; > public: > A() // warning > { > func(foo); > } > }; > - I think this should be considered a false positive. But we may just consider > foo assigned as soon as a variable holds a reference to it, just to keep things > sane. > class A { > int foo; > public: > A() // warning > { > int & bar = foo; > bar = 0; > } > }; Ok, how far should we go? There are a lot of ways in which class members may be initialized. Here are some of them: 1. Only declaration of function is known: void func(int & a); // func() is defined in some other place class A { int foo; public: A() // need warning on foo? { func(foo); } }; 2. How should we process pointers? class A { int foo; public: A() // need warning on foo? { int* bar = &foo; bar++; // Pointer was changed. *bar = 0; } }; 3. What about code with semantic errors? void func(const int & a) { a = 0; // Yes, this code will not compile, but how checker should process it? } class A { int foo; public: A() // need warning on foo? { func(foo); } }; 4. Should we track execution flow? class A { int foo; public: A() // need warning on foo? { if (someFunction()) foo = 0; } }; So, I can cover cases you mentioned above but it seems a lot of other uncovered cases are left. Don't you think that we should consider only base cases?
(In reply to comment #17) > If B constructor does not call A constructor directly the default A constructor > will be called. So warnings will be added to the following correct case: Duh, yes sorry. > Ok, how far should we go? There are a lot of ways in which class members may be > initialized. ... > So, I can cover cases you mentioned above but it seems a lot of other uncovered > cases are left. Don't you think that we should consider only base cases? I think we should only consider base cases but if we encounter a complex case for a field it should default the field to initialized, not uninitialized. I wonder if we could do something like CSearchUtil.isWriteOccurrence which is being used for marking write occurrences? CSearchUtil.isWriteOccurrence is in cdt.ui but we only need the CPP case so we can do something like bool isWrite = ((CPPVariableReadWriteFlags.getReadWriteFlags(node) & PDOMName.WRITE_ACCESS) != 0); So it would go something like this: 1. Build constructorMap as we do right now 2. Visit all IASTName in the constructor definition 3. Check the write access, remove from constructorMap if it is > 1. Only declaration of function is known: > void func(int & a); // func() is defined in some other place > class A { > int foo; > public: > A() // need warning on foo? > { > func(foo); > } > }; No warning. I don't think we should look into func definition (like for occurrences, it will be considered write). > 2. How should we process pointers? > class A { > int foo; > public: > A() // need warning on foo? > { > int* bar = &foo; > bar++; // Pointer was changed. > *bar = 0; > } > }; No warning, as soon as we detect int* bar = &foo; (like for occurrences, it will be considered write). > 3. What about code with semantic errors? > void func(const int & a) > { > a = 0; // Yes, this code will not compile, but how checker should process > it? > } > class A { > int foo; > public: > A() // need warning on foo? > { > func(foo); > } > }; Warning. (like for occurrences, it will be considered read) > 4. Should we track execution flow? > class A { > int foo; > public: > A() // need warning on foo? > { > if (someFunction()) > foo = 0; > } > }; No warning. (like for occurrences, it will be considered write).
(In reply to comment #18) > CSearchUtil.isWriteOccurrence is in cdt.ui but we only need the CPP case so we > can do something like > bool isWrite = ((CPPVariableReadWriteFlags.getReadWriteFlags(node) & > PDOMName.WRITE_ACCESS) != 0); ... Oh, I didn't know about it, but it seems it should help. I'll fix your notes and provide a new patch soon.
Created attachment 193246 [details] Fixed checker implementation (see comment for details) Sorry for delay, finally I fixed things described above: 1. CPPVariableReadWriteFlags is used to detect variable modification (it really makes checker much more clever!). 2. resolveBinding() is used instead of getOrResolveBinding(). 3. Foreach loops are used everywhere. 4. Typo in testBaseClassMemberShouldNotBeTakenIntoAccount was fixed. 5. constructorMap was renamed to fieldsInConstructor. 6. A few test cases were added (the examples that were considered as false positive). I have a question: There is a warning on my checker's code: - Discouraged access: The type CPPVariableReadWriteFlags is not accessible due to restriction on required project org.eclipse.cdt.core (and a few similar) How it should be fixed?
(In reply to comment #20) > I have a question: > There is a warning on my checker's code: > - Discouraged access: The type CPPVariableReadWriteFlags is not accessible > due to restriction on required project org.eclipse.cdt.core > (and a few similar) > How it should be fixed? It's unfortunate but I think the checker really benefits a lot from using this. If we start using this in other checkers then we might think about creating API for it. But for now, I think it's OK. In testNoErrorsOnReadingFunctionCall, in the comments of the test code, // 1 warning for: i1. It should be 'for: i'. But it's minor so I'll fix it when I commit. I'll fill a CQ for IP review.
CQ 5099. I think you need to do a similar statement to bug 326269 comment 12.
(In reply to comment #21) > It's unfortunate but I think the checker really benefits a lot from using this. > If we start using this in other checkers then we might think about creating API > for it. But for now, I think it's OK. Eclipse's Quick Fix proposes to add "@SuppressWarnings("restriction")". Should it be added until we decide to create API?
a. I authored 100% of the content I am contributing b. I have the rights to donate the content to Eclipse c. I contribute the content under the EPL
(In reply to comment #23) > (In reply to comment #21) > > It's unfortunate but I think the checker really benefits a lot from using this. > > If we start using this in other checkers then we might think about creating API > > for it. But for now, I think it's OK. > > Eclipse's Quick Fix proposes to add "@SuppressWarnings("restriction")". Should > it be added until we decide to create API? I'd rather keep the warnings as a reminder. It seems most of CDT doesn't suppress the warnings like that, not sure if that's intentional or not.
(In reply to comment #25) > I'd rather keep the warnings as a reminder. It seems most of CDT doesn't > suppress the warnings like that, not sure if that's intentional or not. Ok, thank you for answer.
Are you guys trying to write checker for uninitialized variables? It is huge can of warms, just few notes: - Pick a simple case and try to implement that - In more complex case choose NOT to report error (which include things like templates, multiple inheritance, fields of unknown types, conditional initialization, etc)
(In reply to comment #27) > Are you guys trying to write checker for uninitialized variables? > It is huge can of warms, just few notes: > - Pick a simple case and try to implement that > - In more complex case choose NOT to report error (which include things like > templates, multiple inheritance, fields of unknown types, conditional > initialization, etc) Thanks, we just came up with this. If there are some false positives, please let me know - I'll try to fix them.
tests: - checkMultiErrorsOnLine is same as checkErrorLines method in superclass - checkErrorOnLine same as checkErrorLines with one argument - first and second test case - same code ? - test itself should be added is suite - AutomatedIntegrationSuite False positive: // rather bad coding - protected constructor - class A cannot be instantiate with uninitialized var class A { protected: A(){}; public: int a;}; class C: public A { C() { a = 1; } }; // using setters in constructor class A { int a; A(int a){ setA(a);}; void setA(int a) { this->a = a;} } // unions union U { int a; char b; U(){ a=0; // reports that b is not initialized - not true strictly speaking } }; Expected false negative (should we write tests for them too?) - No default constructor found - Field initialed only on the one of the if branches
(In reply to comment #29) > tests: > - checkMultiErrorsOnLine is same as checkErrorLines method in superclass It is not quite the same. For example: checkMultiErrorsOnLine(3, 5) is the same as checkErrorLines(3, 3, 3, 3, 3), but it seems that checkMultiErrorsOnLine() is more convenient. So, should I remove checkMultiErrorsOnLine() and use checkErrorLines() instead? > - checkErrorOnLine same as checkErrorLines with one argument > - first and second test case - same code ? > - test itself should be added is suite - AutomatedIntegrationSuite Thanks, fixed. > False positive: > // rather bad coding - protected constructor - class A cannot be instantiate > with uninitialized var > class A { protected: A(){}; public: int a;}; > class C: public A { > C() { > a = 1; > } > }; Why it is false positive? There might me another class (may be even in another translation unit) that does not initialize 'a'. Consider another case: void f() { class A { protected: A(){}; public: int a;}; // Warning on 'a' A a; a.a = 1; } Is this false positive too? If you don't agree please, describe more detailed which cases should be handled. > // using setters in constructor > class A { int a; > A(int a){ setA(a);}; > void setA(int a) { this->a = a;} > } Do you think we should check all the methods called from constructor too? > // unions > union U { > int a; > char b; > U(){ > a=0; // reports that b is not initialized - not true strictly speaking > } > }; I think unions should be skipped at all, cause it is quite hard to figure out what field of union were initialized with assignment. > Expected false negative (should we write tests for them too?) > - No default constructor found Maybe it will be better to add warning if there are no constructors in the class and there are a few variables of simple type? > - Field initialed only on the one of the if branches I don't want to add tests that rely on incorrect checker behavior, but if you think they should be added - I'll add them.
> > > > False positive: > > // rather bad coding - protected constructor - class A cannot be instantiate > > with uninitialized var > > class A { protected: A(){}; public: int a;}; > > class C: public A { > > C() { > > a = 1; > > } > > }; > > Why it is false positive? There might me another class (may be even in another > translation unit) that does not initialize 'a'. Consider another case: > void f() > { > class A { protected: A(){}; public: int a;}; // Warning on 'a' > A a; > a.a = 1; > } > Is this false positive too? > If you don't agree please, describe more detailed which cases should be > handled. You cannot have a code like that, you cannot instantiate A because it does not have a public constructor. I am not saying your checker should catch that, it is too complex and bad code style anyway. > > > > // using setters in constructor > > class A { int a; > > A(int a){ setA(a);}; > > void setA(int a) { this->a = a;} > > } > > Do you think we should check all the methods called from constructor too? No, it would be too much > > > > // unions > > union U { > > int a; > > char b; > > U(){ > > a=0; // reports that b is not initialized - not true strictly speaking > > } > > }; > > I think unions should be skipped at all, cause it is quite hard to figure out > what field of union were initialized with assignment. Yes, they should be skipped. > > > > Expected false negative (should we write tests for them too?) > > - No default constructor found > > Maybe it will be better to add warning if there are no constructors in the > class and there are a few variables of simple type? If indexer cannot find the implementation it may be false positive > > > > - Field initialed only on the one of the if branches > > I don't want to add tests that rely on incorrect checker behavior, but if you > think they should be added - I'll add them. Tests should be added for expected behavior, in case it changes - test would fail and it should be inspected.
(In reply to comment #30) > (In reply to comment #29) > > tests: > > - checkMultiErrorsOnLine is same as checkErrorLines method in superclass > > It is not quite the same. For example: checkMultiErrorsOnLine(3, 5) is the same > as checkErrorLines(3, 3, 3, 3, 3), but it seems that checkMultiErrorsOnLine() > is more convenient. So, should I remove checkMultiErrorsOnLine() and use > checkErrorLines() instead? > Sorry it is a different method please disregard my comment
(In reply to comment #31) > > I think unions should be skipped at all, cause it is quite hard to figure out > > what field of union were initialized with assignment. > > Yes, they should be skipped. Fixed. > > > Expected false negative (should we write tests for them too?) > > > - No default constructor found > > > > Maybe it will be better to add warning if there are no constructors in the > > class and there are a few variables of simple type? > > If indexer cannot find the implementation it may be false positive How can it happen? Can you give an example > Tests should be added for expected behavior, in case it changes - test would > fail and it should be inspected. Ok, the tests were added. P.S. I will provide a new patch with fixes soon.
Created attachment 194262 [details] Fixed checker implementation & tests Changes: - unions are skipped - tests were fixed after Alena's review
So what about my patch? Indigo Release is done, migration to git seems to be completed, is it time to commit the patch? What is the status of IP review request? Is there anything I should do (e.g. convert my patch to the new git repository)?
(In reply to comment #35) > So what about my patch? Indigo Release is done, migration to git seems to be > completed, is it time to commit the patch? What is the status of IP review > request? Is there anything I should do (e.g. convert my patch to the new git > repository)? IP review is done. I'll commit soon.
(In reply to comment #36) > IP review is done. I'll commit soon. Ok, thanks!
> // using setters in constructor > class A { int a; > A(int a){ setA(a);}; > void setA(int a) { this->a = a;} > } Sorry, I thought that case was handled in the newest patch. I feel like it's too important to ignore. I think we should do false negatives for cases where constructor calls non-const method or a function that passes 'this' as a reference or pointer, such as: class C; void initObject(C* c); class C { public: int field; C() { initObject(this); } };
(In reply to comment #38) > I think we should do false negatives for cases where > constructor calls non-const method or a function that passes 'this' as a > reference or pointer, such as: > > class C; > void initObject(C* c); > > class C { > public: > int field; > C() { > initObject(this); > } > }; I cannot figure out what wrong with this case. It seems currently checker will print warning on field. Do you think it is not necessary? Or doesn't it print?
(In reply to comment #38) > I think we should do false negatives for cases where constructor calls non-const method... And one more question for non-const method call. Should we check its body or just don't report any warnings on class members if it is found? And what should we do if method body is not available (e.g. it is in cpp-file, but constructor is in header)?
(In reply to comment #40) > (In reply to comment #38) > > I think we should do false negatives for cases where constructor calls non-const method... > > And one more question for non-const method call. Should we check its body or > just don't report any warnings on class members if it is found? And what should > we do if method body is not available (e.g. it is in cpp-file, but constructor > is in header)? I meant we should not report any warnings. I think analyzing the body inside all method/function calls would be excessive, especially if additional ASTs need to be computed. Sorry, I should've added this: void initObject(C* c) { c->field = 0; // false positive } So, I think we should not report any warnings if we find a function call that's passing 'this' as a reference or pointer parameter.
(In reply to comment #41) > So, I think we should not report any warnings if we find a function call that's > passing 'this' as a reference or pointer parameter. It is not hard to implement, but it seems that we'll have too many false negatives. I think it is better to print warning on this case: > class C { > public: > int field; > C() { > initObject(this); > } > }; Cause it is easy to fix it like this: class C { public: int field; C() : field(0) { initObject(this); } }; And it will be more reliable cause if body of initObject() will change in future the variable field will still be initialized. However if we just skip these cases we will miss the initObject() change and field may become uninitialized. What do you think?
(In reply to comment #42) > (In reply to comment #41) > > So, I think we should not report any warnings if we find a function call that's > > passing 'this' as a reference or pointer parameter. > > It is not hard to implement, but it seems that we'll have too many false > negatives. I would rather have the checker work only on simple cases and have false negatives than have false positives and I think that's the approach the other checkers take as well. That said, one thing we could do to minimize the false negatives is to check the function bodies that are in the same AST, to avoid computing additional ASTs. IASTTranslationUnit.getDefinitionsInAST might be of help here to locate the non-const function of interest in the AST then it can be visited to look for the fields. I don't know how fast that would be. For functions defined outside the current AST, there's probably a way to use IIndex.findDefinitions(IBinding binding), IIndexName.getEnclosedNames() and IIndexName.isWriteAccess(). So, there are some options if we want to look inside the function bodies but I feel like the checker would still be useful if we only considered basic cases and did not warn at all when we have such non-const function calls. > I think it is better to print warning on this case: > > class C { > > public: > > int field; > > C() { > > initObject(this); > > } > > }; > > Cause it is easy to fix it like this: > > class C { > public: > int field; > C() : field(0) { > initObject(this); > } > }; > > And it will be more reliable cause if body of initObject() will change in > future the variable field will still be initialized. However if we just skip > these cases we will miss the initObject() change and field may become > uninitialized. What do you think? I don't know about that, I wouldn't want to have to initialize a member twice to make a false positive disappear, especially if it's not a simple constant and it involves other calls. I also feel like this checker should be more about warning when it knows for sure that a member is not initialized and not about it being initialized in a more 'future-proof' way.
(In reply to comment #43) > I don't know about that, I wouldn't want to have to initialize a member twice > to make a false positive disappear, especially if it's not a simple constant > and it involves other calls. The checker checks simple types only (int, pointers, enums, ...), so it seems there should not involve other calls. > I also feel like this checker should be more about > warning when it knows for sure that a member is not initialized and not about > it being initialized in a more 'future-proof' way. Ok, let me implement a preference "Skip constructors with non-constant methods calls" which will be enabled by default and will work as you said. But if user wants to check all constructors, it can disable this option and the checker will work as it currently does. Do you agree with this solution?
Created attachment 199104 [details] Checker implementation that skips constructors with function and non-const method calls (with preference) The patch implementing my proposed solution.
(In reply to comment #44) > (In reply to comment #43) > Ok, let me implement a preference "Skip constructors with non-constant methods > calls" which will be enabled by default and will work as you said. But if user > wants to check all constructors, it can disable this option and the checker > will work as it currently does. Do you agree with this solution? Yes I agree. Patch and tests look good.
Comment on attachment 199104 [details] Checker implementation that skips constructors with function and non-const method calls (with preference) Woops, meant iplog+ not review+
Comment on attachment 193246 [details] Fixed checker implementation (see comment for details) This was the original approved patch. I don't believe the new one has to go through IP review again since it's not that big of a difference really.
Committed to master. Thanks for working on this!
(In reply to comment #49) > Committed to master. Thanks for working on this! Thank you too!
(In reply to comment #38) > class C; > void initObject(C* c); > > class C { > public: > int field; > C() { > initObject(this); > } > }; When I put the constructor definition into the .cpp file C::C(){ initObject(this); } then I still get false positive warnings. Can somebody have a look at this?
I get another false positive warning when I use a simple copy constructor: class C { public: int field; C(); C(const C& toBeCopied) { *this = toBeCopied; }; };
(In reply to comment #51) > then I still get false positive warnings. > Can somebody have a look at this? Can you create a separate bug? I will look at it. Thanks!
(In reply to comment #53) > (In reply to comment #51) > > then I still get false positive warnings. > > Can somebody have a look at this? > > Can you create a separate bug? I will look at it. Thanks! I created bugs #368419 and #368420 to cover the problems mentioned in comment #51 and #52
