[PATCH] D16044: getVariableName() for MemRegion

[Alexander Droste via cfe-commits]

Alexander_Droste added inline comments.

================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:160
@@ -153,1 +159,3 @@
+  /// \returns variable name for memory region
+  std::string getVariableName() const;
 };
----------------
dcoughlin wrote:
> Alexander_Droste wrote:
> > dcoughlin wrote:
> > > I'm not sure that 'getVariableName()' conveys what this function does. The returned name is not always the name of a variable. How do you intend to use this method? How is it different than than printPretty()? Is it for debugging purposes? If so, then perhaps 'getDebugDescription()'? Is it for presentation to the user? Then perhaps 'getDescriptiveName()'?
> > > The returned name is not always the name of a variable.
> > In which cases doesn't this function return the name? Isn't the worst case when `printPretty` can't be called and the string is empty?
> > 
> > The function is intended to return the variable name and collects index variable names / `ConcreteInt`s along the way if the passed region is an `ElementRegion`. So `printPretty` is called on the 'top' region of an array at the end of this function. If the passed region is no `ElementRegion` the functionality is identical to `printPretty`. 
> > 
> > The main motivation to write this function was to include specific indices in a diagnostic if obtainable, like presented in the MPI-Checker testfile  
> > http://reviews.llvm.org/D12761#3c51dfcf.
> > E.g. `Request 'sendReq1[1][7][9]' has no matching nonblocking call.` My intent was not to use the function for debugging purposes.
> > 
> > So should we use `getDescriptiveName` or maybe sth. like `getVariableNameWithIndices`?.
> Can the string ever be something like 'a.f[0][1]' (i.e., with a field name)? Or is it always 'a[0][1]'? If the first is possible I would suggest `getDescriptiveName` -- if not, then `getVariableNameWithIndices` seems fine.
Yes, the string includes the name of the struct.  But this is not due to the while loop in this function but how `printPretty` is defined for `FieldRegion`.  In case of a `FieldRegion` `printPretty` is called for the super-region first before the field declation name gets appended. 
```
void FieldRegion::printPrettyAsExpr(raw_ostream &os) const {
  assert(canPrintPrettyAsExpr());
  superRegion->printPrettyAsExpr(os);
  os << "." << getDecl()->getName();
}
```
I'll go with `getDescriptiveName`.


http://reviews.llvm.org/D16044