[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus marked 6 inline comments as done.
Szelethus added a comment.

I'm about to update the diff, I changed a quite a lot of stuff, so I'm not sure that I'd be able to respond to these inline comments.



================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:31
+/// every other element is a field, and the element that precedes it is the
+/// object that contains it.
+class FieldChainInfo {
----------------
xazax.hun wrote:
> As far as I understand, for every operation, the only relevant contribution of the non-last elements in the chain is the diagnostic message.
> I wonder if you would build a string instead of a FieldRegion chain and only store the last FieldRegion would make this more explicit in the code. 
I experimented with a number of implementations along this idea, but I just couldn't get it to work. Here are my findings:
* `Twine`-s aren't meant to be copied, so using them didn't work out
* `StringRef`s for some reason get invalidated by the time it'd make a call to `FieldChainInfo::getAsString()`
* I didn't like the idea of storing the string because it'd not only impact performance greatly, but it'd also make the code a lot harder to understand, as opposed to making it more explicit

I did however try to make the implementation more easy to understand.


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:35
+
+  FieldChain Chain;
+  // If this is a fieldchain whose last element is an uninitialized region of a
----------------
xazax.hun wrote:
> I was wondering, do you need the chain at all? I think a field region might be sufficient. The enclosing object of the field should be accessible by querying the super region of the field region.
I tried it, but that approach made it impossible to include pointers and references in the fieldchain.


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:218
+
+  std::string WarningMsg = std::to_string(UninitFields.size()) +
+                           " uninitialized field" +
----------------
whisperity wrote:
> Maybe one could use a Twine or a string builder to avoid all these unneccessary string instantiations? Or `std::string::append()`?
Doesn't move semantics take care of that? I'm not too sure on this one.


https://reviews.llvm.org/D45532