NoQ added a comment.
I showed the bug mentioned in https://reviews.llvm.org/D49058 to a friend who
didn't do much C++ recently, for a fresh look, and he provided a bunch of
interesting feedback by explaining the way he didn't understand what the
analyzer was trying to say.
1. When we call `c_str()`, the pointer is not dangling yet, not until the
destructor or realloc is called. He didn't understand the report because he was
trying to figure out why do we think the pointer is already dangling.
2. A generic "use after free" warning on the return site is confusing because
the user would expect to see an actual "use" instead of just passing it around.
We should be more specific, i.e. "Deallocated pointer returned to the caller".
3. We mention that there's a destructor, but the destructor is hard to see.
Knowing the type of the destroyed object would help. Knowing that it's a
temporary object would help.
4. The whole idea of "string has a buffer that would be destroyed when the
string is destroyed and we shouldn't pass the pointer around" needs to be
explained all together, rather than separated into different diagnostic pieces.
The user needs to be somehow informed that this is how `std::string` operates
because he doesn't necessarily know that.
================
Comment at: test/Analysis/dangling-internal-buffer.cpp:176
+ c = s.c_str(); // expected-note {{Pointer to dangling buffer was obtained
here}}
+ s.clear(); // expected-note {{Method call is allowed to invalidate the
internal buffer}}
+ consume(c); // expected-warning {{Use of memory after it is freed}}
----------------
rnkovacs wrote:
> dcoughlin wrote:
> > What do you think about explicitly mentioning the name of the method here
> > when we have it? This will make it more clear when there are multiple
> > methods on the same line.
> >
> > I also think that instead of saying "is allowed to" (which raises the
> > question "by whom?") you could make it more direct.
> >
> > How about:
> > "Inner pointer invalidated by call to 'clear'"
> >
> > or, for the destructor "Inner pointer invalidated by call to destructor"?
> >
> > What do you think?
> >
> > If you're worried about this wording being to strong, you could weaken it
> > with a "may be" to:
> >
> > "Inner pointer may be invalidated by call to 'clear'"
> >
> >
> I like these, thanks! I went with the stronger versions now, as they seem to
> fit better with the warnings themselves.
> "Inner pointer invalidated by call to 'clear'"
I think the word "invalidated" may be confusing, how about "reallocated"? And
"deallocated" in case of destructors.
https://reviews.llvm.org/D49360
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
