[PATCH] Add new diagnostic to check for bad member function calls in mem-initializer-lists.

Richard Trieu rtrieu at google.com
Tue Dec 9 18:42:28 PST 2014


Besides the comments above, see http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface on how to get a full diff.  Giving Phabricator a full diff makes reviewing easier.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:2643
@@ +2642,3 @@
+class DiagnoseMemberCallVisitor
+    : public RecursiveASTVisitor<DiagnoseMemberCallVisitor> {
+  Sema &S;
----------------
There's multiple visitors in Clang.  The RecursiveASTVisitor will visit every AST node, including unevaluated contexts such as sizeof expressions.  It would be better to use an EvaluatedExprVisitor which ignores unevaluated expressions.  Since there already is an EvaluatedExprVisitor for member initializers, it would be a better idea to try to add to that visitor instead of writing your own to minimize how often these expressions are traversed.

The best place to look is HandleMemberExpr inside UninitializedFieldVisitor.  This is were all MemberExpr's that need to be checked are funneled into.

================
Comment at: test/SemaCXX/member-call-before-bases-init.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -fsyntax-only -Wuninitialized -verify %s
+
----------------
Merge this test case with uninitalized.cpp.  The warnings here and in uninitalized.cpp come from the same warning group and should be kept together in test cases.

================
Comment at: test/SemaCXX/member-call-before-bases-init.cpp:31
@@ +30,3 @@
+public:
+  D() : C(f()), // expected-warning{{member function 'f' called before all base classes were initialized}}
+        i(f()) {}  // no-warning
----------------
f() is from class B, which is fully initialized when class C is being constructed.  There is no need for a warning here.

================
Comment at: test/SemaCXX/member-call-before-bases-init.cpp:33
@@ +32,2 @@
+        i(f()) {}  // no-warning
+};
----------------
Add a test case using an unevaluated context to check that no warning is emitted.  For example, sizeof(f()) in an initializer.


================
Comment at: test/SemaCXX/uninitialized.cpp:1326
@@ -1325,2 +1325,3 @@
   // expected-warning at -1 {{base_class_access::A' is uninitialized when used here to access 'base_class_access::A::foo'}}
+  // expected-warning at -2 {{member function 'foo' called before all base classes were initialized}}
 };
----------------
Don't trigger the warning here.  The existing warning is good enough and the new warning doesn't add anything new here.

================
Comment at: test/SemaCXX/uninitialized.cpp:1342
@@ -1340,2 +1341,3 @@
   // expected-warning at -1 {{base_class_access::A' is uninitialized when used here to access 'base_class_access::A::foo'}}
+  // expected-warning at -2 {{member function 'foo' called before all base classes were initialized}}
 };
----------------
Same as above.

http://reviews.llvm.org/D6561






More information about the cfe-commits mailing list