[PATCH] D16044: getVariableName() for MemRegion

[Alexander Droste via cfe-commits]

Alexander_Droste added a comment.

Hi Devin,
thanks for taking the time! Yes, this is kind of implicitly tested by the MPI patch but I agree that it is necessary to add tests to MemRegion.


================
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:
> 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`?.

================
Comment at: lib/StaticAnalyzer/Core/MemRegion.cpp:635
@@ -634,1 +634,3 @@
 
+std::string MemRegion::getVariableName() const {
+  std::string VariableName;
----------------
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.


http://reviews.llvm.org/D16044