[PATCH] misc-uninitialized-field

[Alexander Kornienko]

================
Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:14
@@ +13,3 @@
+#include <unordered_set>
+#include <sstream>
+#include <iterator>
----------------
jbcoe wrote:
> alexfh wrote:
> > #includes should be sorted (e.g. using the llvm-include-order check, `clang-tidy -fix` should do the trick).
> I've applied my understanding of http://llvm.org/docs/CodingStandards.html#include-style. The clang-tidy check you suggested running made no changes to the code.
> 
> Please let me know if I've misunderstood anything.
The comment related to the version where `unordered_set`, `sstream` and `iterator` were included in this specific order. Now that the last two are gone, there's no issue.

================
Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:24
@@ +23,3 @@
+
+static bool fieldRequiresInit(const clang::FieldDecl *f) {
+  if (f->getType()->isPointerType()) 
----------------
jbcoe wrote:
> alexfh wrote:
> > Variable names should start with a capital letter.
> I'll change this. 
> Out of curiosity, why is this LLVM's way of naming variables? I've not seen this style used elsewhere.
I don't know if there was any reason to choose this specific style. Maybe ask Chris Lattner? ;)

================
Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:56
@@ +55,3 @@
+    
+    for (clang::CXXCtorInitializer *Init : Ctor->inits()) {
+      const FieldDecl *MemberField = Init->getMember();
----------------
jbcoe wrote:
> alexfh wrote:
> > jbcoe wrote:
> > > alexfh wrote:
> > > > What about fields initialized in the constructor body?
> > > My check does not take that into account. I think it's hard to do in general so I decided to let the user decide to ignore the check and proposed change. I presume that the optimiser is smart enough to do the right thing if a variable is obviously initialized in the constructor body and constructor field initializers.
> > > 
> > What about this?
> I got confused by comment submission. Comment added.
This decision needs to be documented in the class comment. And it may make sense to look at the number of false positives resulting from specifically this case and estimate how many of those could be avoided by just looking at which fields are assigned inside the constructor body.

================
Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:70
@@ +69,3 @@
+
+    std::stringstream ss;
+    for (const auto MemberField : MemberFields) 
----------------
jbcoe wrote:
> alexfh wrote:
> > alexfh wrote:
> > > In LLVM code, `llvm::raw_string_ostream` or `llvm::raw_svector_ostream` are used instead of `std::stringstream`.
> > Did you miss this comment?
> I'm using a string instead. llvm::raw_string_ostream takes a string ref as a constuctor argument and the code is simpler if I just use the string directly.
`llvm::raw_string_ostream` is preferred for efficiency reasons. Your current version creates temporary strings on each iteration. You could prevent this by appending the two strings separately or by using `raw_string_stream`. The latter seems like a more elegant solution to me.

================
Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:72
@@ +71,3 @@
+    assert(!Replacement.empty());
+    Replacement.pop_back(); // remove trailing ','
+
----------------
nit: Capitalization. Trailing period.

================
Comment at: clang-tidy/misc/UninitializedFieldCheck.h:18
@@ +17,3 @@
+
+/// \brief Checks that builtin or pointer fields are initialized by explicit constructors.
+class UninitializedFieldCheck : public ClangTidyCheck {
----------------
80 columns

http://reviews.llvm.org/D10553

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/