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