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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
