Alexander_Droste 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;
};
dcoughlin wrote:
> Alexander_Droste
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:
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
dcoughlin added a comment.
Hi Alexander,
Some comments in line. Also, I don't see any tests. Is this code tested by your
MPI patch?
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:160
@@ -153,1 +159,3 @@
+ /// \returns variable name for memory region
+
xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.
It looks good to me. Devin could you take a look at this as well?
http://reviews.llvm.org/D16044
___
cfe-commits mailing list
cfe-commits@l
Alexander_Droste updated this revision to Diff 48959.
Alexander_Droste added a comment.
- removed `reverse`
- fixed twine usage
I think `VariableName.insert(VariableName.size() - 1, ArrayIndices);` is needed
here.
varName, indices -> 'varname[...]' If append would be used, the second single
qu
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Core/MemRegion.cpp:672
@@ +671,3 @@
+// Combine variable name with indices.
+VariableName.insert(VariableName.size() - 1, ArrayIndices);
+ }
nit: this is in fact an append. It is cleaner inst
xazax.hun added a comment.
Since we went with the twine solution, I think we can get rid of the reverse
stuff.
Note that:
Twine should be used like: (Twine(A) + B + C).str()
So the operator+ has twine as one of the operands.
Once these are fixed, it looks good to me.
Do you have commit access o
Alexander_Droste updated this revision to Diff 48953.
Alexander_Droste added a comment.
- remove string idx variable -> remove string copy assignments
- use Twine to reduce temporary string objects, built during concatenation
Is `ArrayIndices = llvm::Twine(ArrayIndices + "]" + intValAsString.str(
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Core/MemRegion.cpp:636
@@ +635,3 @@
+std::string MemRegion::getVariableName() const {
+ std::string VariableName{""};
+ std::string ArrayIndices{""};
Small nit, I prefer to call the default construc
Alexander_Droste updated this revision to Diff 48949.
Alexander_Droste added a comment.
- Changed `idx = R->getVariableName();` to `idx = ER->getVariableName();` as
it better describes what is happening on a semantic level.
http://reviews.llvm.org/D16044
Files:
include/clang/StaticAnalyzer
xazax.hun added a comment.
In http://reviews.llvm.org/D16044#360737, @Alexander_Droste wrote:
> Hi Gábor,
> Is it really possible to reduce string copying in this case, with the help
> of `llvm::Twine`?
> The problem with `llvm::Twine` seems to me that it cannot be modified after
> initialisa
Alexander_Droste updated this revision to Diff 48940.
Alexander_Droste added a comment.
Changes:
- remove `if (VariableName.size() && VariableName.back() == '\'')` check
- call `getVariableName` for index if Region is no `ConcreteInt`
Hi Gábor,
sorry about the delay! Here's the updated patch.
Is
xazax.hun added a comment.
What is the status if this? Do you need some help with any of the
modifications? I'd love to see this patch in the repository soon.
http://reviews.llvm.org/D16044
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ht
xazax.hun added inline comments.
Comment at: tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:662
@@ +661,3 @@
+else if (auto SV =
+ER->getIndex().getAs()) {
+llvm::SmallString<8> buf;
Alexander_Droste wrote:
> Alexander_Droste wrote:
> > xaza
Alexander_Droste added a comment.
Comment at: tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:662
@@ +661,3 @@
+else if (auto SV =
+ER->getIndex().getAs()) {
+llvm::SmallString<8> buf;
Alexander_Droste wrote:
> xazax.hun wrote:
> > These a
Alexander_Droste added inline comments.
Comment at: tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:662
@@ +661,3 @@
+else if (auto SV =
+ER->getIndex().getAs()) {
+llvm::SmallString<8> buf;
xazax.hun wrote:
> These are the only cases that ca
xazax.hun added inline comments.
Comment at: tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:660
@@ +659,3 @@
+}
+
+else if (auto SV =
The else should go into the same line as the closing }.
Comment at: tools/clang/lib/StaticAnalyzer/Co
Alexander_Droste updated this revision to Diff 45239.
Alexander_Droste added a comment.
- reduce copying
- differentiate ER->getIndex().getAs<> cases
How about this?
http://reviews.llvm.org/D16044
Files:
tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
tools/clang/li
Alexander_Droste marked an inline comment as done.
Comment at: tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:653
@@ +652,3 @@
+llvm::SmallString<2> intValAsString;
+IndexInArray.toString(intValAsString);
+std::string idx{intValAsString.begin(), intValAsString.end()
xazax.hun added inline comments.
Comment at: tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:653
@@ +652,3 @@
+llvm::SmallString<2> intValAsString;
+IndexInArray.toString(intValAsString);
+std::string idx{intValAsString.begin(), intValAsString.end()};
---
Alexander_Droste marked 2 inline comments as done.
Comment at: tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:643
@@ +642,3 @@
+ const clang::ento::MemRegion *R = this;
+ llvm::SmallString<50> buf;
+ llvm::raw_svector_ostream os(buf);
You're right, 200 was a
Alexander_Droste updated this revision to Diff 44476.
Alexander_Droste added a comment.
- reduced buffer size for variable name
- simplified `R->getAs()->getSuperRegion()` to
`ER->getSuperRegion()`.
http://reviews.llvm.org/D16044
Files:
tools/clang/include/clang/StaticAnalyzer/Core/PathSensi
xazax.hun added inline comments.
Comment at: tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:643
@@ +642,3 @@
+ const clang::ento::MemRegion *R = this;
+ llvm::SmallString<200> buf;
+ llvm::raw_svector_ostream os(buf);
Do you think 200 is a reasonable size he
24 matches
Mail list logo
