vitalybuka added a comment.
LGTM with some nits
================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:124
+
+namespace {
+
----------------
maybe remove all static about and extend namespace {}
================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:136
+
+static ShadowMapping getShadowMapping(Triple &TargetTriple, int LongSize) {
+ ShadowMapping Mapping;
----------------
this could be ShadowMapping::ShadowMapping()
================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:136
+
+static ShadowMapping getShadowMapping(Triple &TargetTriple, int LongSize) {
+ ShadowMapping Mapping;
----------------
vitalybuka wrote:
> this could be ShadowMapping::ShadowMapping()
params are not used?
================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:151-155
+ if (TargetTriple.isOSEmscripten()) {
+ return kHeapProfEmscriptenCtorAndDtorPriority;
+ } else {
+ return kHeapProfCtorAndDtorPriority;
+ }
----------------
For consistency
or even "?:"
================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:194
+
+private:
+ void initializeCallbacks(Module &M);
----------------
private/public can be reordered and struct converted to class
================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:386
+ auto *F = CI->getCalledFunction();
+ if (F && (F->getName().startswith("llvm.masked.load.") ||
+ F->getName().startswith("llvm.masked.store."))) {
----------------
F->getIntrinsicID() == Intrinsic::masked_load
F->getIntrinsicID() == Intrinsic::masked_store
================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:614
+ for (auto &Inst : BB) {
+ if (isInterestingMemoryAccess(&Inst, &IsWrite, &TypeSize, &Alignment) ||
+ isa<MemIntrinsic>(Inst))
----------------
probably nicer to put output param into struct {} and rely on RVO
as is it's hard to track if they are initialized
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85948/new/
https://reviews.llvm.org/D85948
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
