vitalybuka accepted this revision. vitalybuka added inline comments.
================ Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:409 + // analysis. + // This is so we don't need to plumb TargetTriple all the way to here. + if (mightUseStackSafetyAnalysis(DisableOptimization)) ---------------- I think this function without comment is clear enough. But up to you. ================ Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:410 + Dummy.setArch(Triple::aarch64); + if (shouldUseStackSafetyAnalysis(Dummy, DisableOptimization)) + AU.addRequired<StackSafetyGlobalInfoWrapperPass>(); ---------------- fmayer wrote: > fmayer wrote: > > vitalybuka wrote: > > > Why not just: > > > ``` > > > if (!DisableOptimization || ClUseStackSafety) > > > AU.addRequired<StackSafetyGlobalInfoWrapperPass>(); > > > ``` > > Because `ClUseStackSafety` defaults to true, so this will be quite useless > > except if the user explicitly sets it to false. So then you end up > > duplicating the logic in `shouldUseStackSafetyAnalysis`, which is why I > > went for the Dummy (thinking about it, we don't actually need to setArch on > > it). > How about this (mightUseStackSafetyAnalysis)? This seems like a nice tradeoff > without duplication of the logic. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105703/new/ https://reviews.llvm.org/D105703 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
