[PATCH] D16044: getDescriptiveName() for MemRegion

[Anna Zaks via cfe-commits]

zaks.anna added inline comments.

================
Comment at: lib/StaticAnalyzer/Core/MemRegion.cpp:653
@@ +652,3 @@
+    // name by calling 'getDescriptiveName' recursively.
+    else {
+      std::string Idx = ER->getDescriptiveName(false);
----------------
xazax.hun wrote:
> Alexander_Droste wrote:
> > I wasn't able to build a test case yet for which the analyzer could not determine the constant value. Is there a way to trick the analyzer so that the else case is used ? Then I could test for something like `'sendReq1[a][7][b]'`.
> You can try use a value returned from a function that has an unknown body. E.g.:
> 
> int getUnknown();
> 
> void f() {
>   int a = getUnKnown();
> }
What happens when you try 'sendReq1[a][7][b]'? Does it know the values for "a" and "b" for some reason? If 'a' would be an input parameter and the analyzer did not see a call site, it won't know the value of 'a'.

================
Comment at: test/Analysis/MemRegion.cpp:3
@@ +2,3 @@
+
+#include "MPIMock.h"
+
----------------
Alexander_Droste wrote:
> The problem about these tests is that they introduce a cyclic commit dependency. MPI-Checker depends on `getDescriptiveName`. `getDescriptiveName` depends on MPI-Checker because of the tests.
> 
> Further, MPI-Checker depends on this function:
> 
> ```
> SourceRange MemRegion::sourceRange() const {
>   const VarRegion *const VR = dyn_cast<VarRegion>(this->getBaseRegion());
>   const FieldRegion *const FR = dyn_cast<FieldRegion>(this);
>   const CXXBaseObjectRegion*const COR = dyn_cast<CXXBaseObjectRegion>(this);
> 
>   // Check for more specific regions first.
>   // FieldRegion
>   if (FR) {
>     return FR->getDecl()->getSourceRange();
>   }
>   // CXXBaseObjectRegion
>   else if (COR) {
>     return COR->getDecl()->getSourceRange();
>   }
>   // VarRegion
>   else if (VR) {
>     return VR->getDecl()->getSourceRange();
>   }
>   // Return invalid source range (can be checked by client).
>   else {
>     return SourceRange{};
>   }
> }
> ```
> Initially, my idea was to submit the `sourceRange` patch after `getDescriptiveName`. Maybe it would be most convenient, to include the `sourceRange` function into this patch and finally commit all connected patches in one go.
> getDescriptiveName depends on MPI-Checker because of the tests.

Would committing the tests separately fix the  cyclic commit dependency problem? (Though, I'd still prefer to review them along with this patch.)


http://reviews.llvm.org/D16044