[PATCH] D47135: [analyzer][WIP] A checker for dangling string pointers in C++

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

This looks great, i think we should make a single super simple mock test and commit this.

@MTC, i really appreciate your help!



================
Comment at: lib/StaticAnalyzer/Checkers/DanglingStringPointerChecker.cpp:59
+  QualType RegType = TypedR->getValueType();
+  if (RegType.getAsString() != "std::string")
+    return;
----------------
MTC wrote:
> A little tip, there are other string types besides `std::string`, like `std::wstring`, which can be added in the future. See [[ http://en.cppreference.com/w/cpp/string/basic_string | basic_string ]].
Yup, the current check should work in many cases, but it should be better to compare the class name to `basic_string`.

I'm also worried about various sorts of `std::__1::string`s (an inline namespace in libc++).


================
Comment at: lib/StaticAnalyzer/Checkers/DanglingStringPointerChecker.cpp:64
+
+  if (Call.isCalled(CStrFn)) {
+    SymbolRef RawPtr = Call.getReturnValue().getAsSymbol();
----------------
It has long been planned to extend `isCalled` to C++ methods, i.e. something like this:
```
DanglingStringPointerChecker() : CStrFn("std::string::c_str") {}

...

void DanglingStringPointerChecker::checkPostCall(const CallEvent &Call,
                                                 CheckerContext &C) const {
  // Skip the class check.
  if (Call.isCalled(CStrFn)) {
    ...
  }

  ...
}
```
Still looking for volunteers^^


================
Comment at: lib/StaticAnalyzer/Checkers/DanglingStringPointerChecker.cpp:71
+
+  if (dyn_cast<CXXDestructorCall>(ICall)) {
+    if (State->contains<RawPtrMap>(TypedR)) {
----------------
MTC wrote:
> `CXXDestructorCall::classof(const CallEvent *CA)` can also be used here. 
`isa`.


================
Comment at: lib/StaticAnalyzer/Checkers/RegionState.h:18
+
+namespace region_state {
+
----------------
MTC wrote:
> I'm not sure whether `region_state` follows the naming conventions of LLVM coding standards. Can someone answer this question?
I think it does, cf. `namespace ast_matchers`.

The name should be more specific though, a lot of checkers track "region states". Maybe "allocation_state"?


Repository:
  rC Clang

https://reviews.llvm.org/D47135