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

[Reka Kovacs via Phabricator via cfe-commits]

rnkovacs added inline comments.


================
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}}
----------------
dcoughlin wrote:
> 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".
I feel like I should also retitle the checker to `DanglingInnerBuffer` to remain consistent. What do you think?


================
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}}
----------------
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.


https://reviews.llvm.org/D49360