<div dir="ltr"><div dir="ltr">Hi!<br><br>Sorry for my late response, I'm currently too busy to start tinkering with this checker, but the feedback is very much appreciated! </div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, 4 May 2019 at 16:57, Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
The results of the UninitializedObject checker significantly overlap<br>
with the clang-tidy checker cppcoreguidelines-pro-type-member-init.<br>
When both checkers trigger, messages from<br>
cppcoreguidelines-pro-type-member-init are significantly more clear<br>
and less noisy.<br>
<br>
$ cat /tmp/t.cc<br>
struct Foo {<br>
Foo() : y(0) {}<br>
int x;<br>
int y;<br>
};<br>
void entry1() {<br>
Foo x;<br>
}<br>
<br>
$ ./bin/clang-tidy -checks="-*,clang-analyzer*" /tmp/t.cc<br>
/tmp/t.cc:2:17: warning: 1 uninitialized field at the end of the<br>
constructor call [clang-analyzer-optin.cplusplus.UninitializedObject]<br>
Foo() : y(0) {}<br>
^<br>
/tmp/t.cc:3:7: note: uninitialized field 'this->x'<br>
int x;<br>
^<br>
/tmp/t.cc:7:7: note: Calling default constructor for 'Foo'<br>
Foo x;<br>
^<br>
/tmp/t.cc:2:17: note: 1 uninitialized field at the end of the constructor call<br>
Foo() : y(0) {}<br>
^<br>
<br>
$ ./bin/clang-tidy -checks="-*,cppcoreguidelines-pro-type-member-init" /tmp/t.cc<br>
/tmp/t.cc:2:3: warning: constructor does not initialize these fields:<br>
x [cppcoreguidelines-pro-type-member-init]<br>
Foo() : y(0) {}<br>
^<br>
<br>
Note how cppcoreguidelines-pro-type-member-init packs the same<br>
information into one message. The primary message from<br>
UninitializedObject checker says there's "1 uninitialized field". As a<br>
user, my first question is "which?" -- and to figure it out I have to<br>
parse the rest of the output, where two notes out of three have<br>
completely irrelevant information. There's no way to call this<br>
constructor in a way that will initialize `Foo::x`, so why print all<br>
that? Also, the phrasing is passive, I like the phrasing from<br>
cppcoreguidelines-pro-type-member-init much better.<br>
<br></blockquote><div><br></div><div>When I use CodeChecker to look at the results, I prefer the notes. However, having different report styles for different outputs (text, plist, etc) would be great!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
cppcoreguidelines-pro-type-member-init does not trigger on templates,<br>
while UninitializedObject checker does. For example:<br>
<br>
template<typename T><br>
struct Foo {<br>
Foo() : y(0) {}<br>
T x;<br>
T y;<br>
};<br>
void entry1() {<br>
Foo<int> x;<br>
}<br>
<br>
It is unclear why cppcoreguidelines-pro-type-member-init decided to<br>
not look into templates.<br>
<br>
One source of false positives for the UninitializedObject checker are<br>
`Init()` methods, for example:<br>
<br>
bool ReadIntFromFile(int *x);<br>
struct Foo {<br>
Foo() : y(0) {}<br>
int x;<br>
int y;<br>
bool Init() {<br>
return ReadIntFromFile(&x);<br>
}<br>
};<br>
bool entry1() {<br>
Foo x;<br>
return x.Init();<br>
}<br>
<br></blockquote><div><br></div><div>Having to remember that you have to call an init function is a good example IMO why we really should emit a report in this case. What kind of heuristic would work well here? "If the class has a method named Init(), temporarily suppress the report, and emit it when the variable goes out of scope without the Init() function being called"? Can the clang-tidy checker deal with this?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
It is also unclear how to annotate member variables that are<br>
intentionally left uninitialized:<br>
<br>
struct Foo {<br>
Foo() : has_cached_value(false) {}<br>
bool has_cached_value;<br>
int cached_value;<br>
};<br>
bool entry1() {<br>
Foo x;<br>
}<br>
<br></blockquote><div><br></div><div>That's a very valid concern.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
In many cases it is possible to rewrite the code using std::optional,<br>
but not everywhere.<br>
<br>
Another source of false positives is the `is_valid` pattern:<br>
<br>
bool ReadIntFromFile(int *x);<br>
struct Foo {<br>
Foo() {<br>
int temp;<br>
if (!ReadIntFromFile(&temp)) {<br>
is_valid = false;<br>
return;<br>
}<br>
x = temp;<br>
is_valid = true;<br>
}<br>
int x;<br>
bool is_valid;<br>
};<br>
void entry1() {<br>
Foo x;<br>
}<br>
<br></blockquote><div><br></div><div>Hmmm, interesting. A better guarded field analysis (one of the options for the checker) might be able to deal with this. In this specific case, x is public, so the danger is real, but if it wasn't, and was guarded with assert(is_valid);, I see why we shouldn't report it.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Inlining in the Clang Static Analyzer helps the UninitializedObject<br>
checker to see through abstractions. In the following example<br>
UninitializedObject checker is silent (as it should be), while<br>
cppcoreguidelines-pro-type-member-init complains:<br>
<br>
struct Foo {<br>
Foo() {<br>
Unset();<br>
}<br>
void Unset() {<br>
has_cached_value = false;<br>
x = 0;<br>
}<br>
bool has_cached_value;<br>
int x;<br>
};<br>
void entry1() {<br>
Foo x;<br>
}<br>
<br>
To summarize, I have two findings.<br>
<br>
First, I haven't found a case where taking the call site context into<br>
account helps this checker. Fundamentally, this check is about<br>
assuming nothing at the entry to the constructor, and checking a<br>
strong postcondition (all fields are initialized) at the exit.<br>
<br>
Second, the `Init()` and `is_valid` patterns are fundamentally at odds<br>
with this checker, because it does not try to detect an uninitialized<br>
memory read, it tries to detect a postcondition violation at the<br>
constructor exit.<br>
<br>
Hope this helps.<br></blockquote><div><br></div><div>Yes it does, thank you for everything! :) I'll definitely make some improvements when I'll have some spare time.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Dmitri<br>
<br>
-- <br>
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if<br>
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com" target="_blank">gribozavr@gmail.com</a>>*/<br>
</blockquote></div></div>