Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
LGTM if you agree with my changes
| llvm/include/llvm/Analysis/StackSafetyAnalysis.h | ||
|---|---|---|
| 81 | "&I" for consistency | |
| llvm/lib/Analysis/StackSafetyAnalysis.cpp | ||
| 137 | you missed my comment about auto vs tie. this will work: std::map<const Instruction *, /*remove const*/ ConstantRange> Accesses;
...
void addRange(const Instruction *I, const ConstantRange &R) {
auto Ins = Accesses.emplace(I, R);
if (!Ins.second)
Ins.first->second = unionNoWrap(Ins.first->second, R);
updateRange(R);
} | |
| 232–233 | for consistency with line above | |
| llvm/lib/Analysis/StackSafetyAnalysis.cpp | ||
|---|---|---|
| 360 | I see why you need "break" instead of "return" in these cases. Could you please to add a test-case sensitive to this part of the patch. Please cover at least few of these breaks. | |
LGTM with Vitaly's comment about test coverage
| llvm/include/llvm/Analysis/StackSafetyAnalysis.h | ||
|---|---|---|
| 81 | I realized that this header does not define what "safe" means anywhere. Especially important for this method. Could you clarify that this means the access is proven to be inbounds of a live alloca? | |
| llvm/lib/Analysis/StackSafetyAnalysis.cpp | ||
| 819 | "UnsafeAccesses" ? | |
| llvm/lib/Analysis/StackSafetyAnalysis.cpp | ||
|---|---|---|
| 360 | Added tests covering most of the return -> break cases. | |
name
| llvm/lib/Analysis/StackSafetyAnalysis.cpp | ||
|---|---|---|
| 819–836 | Sorry, I was sure I did this, but must have messed up the upload somehow. | |
"&I" for consistency