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