[PATCH] D49360: [analyzer] Add support for more basic_string API in DanglingInternalBufferChecker

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a comment.

It is really nice to see this checker take shape! Some drive by diagnostic comments in line.



================
Comment at: test/Analysis/dangling-internal-buffer.cpp:175
   std::string s;
-  {
-    c = s.c_str();
-  }
-  consume(c); // no-warning
+  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}}
----------------
In other parts of clang we use the term "inner pointer" to mean a pointer that will be invalidated if its containing object is destroyed https://clang.llvm.org/docs/AutomaticReferenceCounting.html#interior-pointers. There are existing attributes that use this term to specify that a method returns an inner pointer.

I think it would be good to use the same terminology here. So the diagnostic could be something like "Dangling inner pointer obtained here".


================
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}}
----------------
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'"




https://reviews.llvm.org/D49360