[PATCH] D16517: ClangTidy check to flag uninitialized builtin and pointer fields.

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:88
@@ +87,3 @@
+    if (InitializerBefore != nullptr)
+      return InitializerBefore->getRParenLoc().getLocWithOffset(1);
+    auto StartLocation = InitializerAfter != nullptr
----------------
Maybe Lexer::getLocForEndOfToken?

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:94
@@ +93,3 @@
+        lexer_utils::getPreviousNonCommentToken(Context, StartLocation);
+    return Token.getLocation().getLocWithOffset(Token.getLength());
+  }
----------------
Maybe Lexer::getLocForEndOfToken?

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:175
@@ +174,3 @@
+  // Ignore code in macros since we can't place the fixes correctly.
+  if (Ctor->getLocStart().isMacroID())
+    return;
----------------
We still can output a warning, just be careful with fixits. Please also add a test for macros to illustrate the behavior. Two cases are interesting: code in a macro body and code in a macro argument.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:205
@@ +204,3 @@
+      Diag << FixItHint::CreateInsertion(
+          Field->getSourceRange().getEnd().getLocWithOffset(
+              Field->getName().size()),
----------------
Maybe Lexer::getLocForEndOfToken?

================
Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:178
@@ +177,3 @@
+
+  SmallPtrSet<const FieldDecl *, 16> FieldsToInit;
+  fieldsRequiringInit(MemberFields, FieldsToInit);
----------------
flx wrote:
> alexfh wrote:
> > What about this? (relates to line 184 now)
> My comment on this was not submitted, just in case. Here it is again:
> 
> I think he idea is to return immediately if we're delegating to another constructor here. Am I misunderstanding the API call?
> 
> The important part is that we return in that case.
I see now. A comment explaining why this is done would be helpful here.


http://reviews.llvm.org/D16517