[PATCH] D16044: getVariableName() for MemRegion

[Devin Coughlin via cfe-commits]

dcoughlin 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;
 };
----------------
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.

================
Comment at: lib/StaticAnalyzer/Core/MemRegion.cpp:635
@@ -634,1 +634,3 @@
 
+std::string MemRegion::getVariableName() const {
+  std::string VariableName;
----------------
Alexander_Droste wrote:
> dcoughlin wrote:
> > It seems like this could potentially be simplified by adding a recursive helper that threads the output stream and appends to it after the base case has been hit. You could also modify prettyPrint() to take a flag indicating whether it should be quoted or not (and defaulting to true). What do you think?
> Modifying `printPretty` seems like a nice solution to me. If you don't prefer the other idea, I would go this route.
Sounds good to me!


http://reviews.llvm.org/D16044