[PATCH] D134097: [BOLT][NFC] Refactor creation of symbol+addend references

Mon Oct 10 17:27:28 PDT 2022

rafauler added a comment.

Thanks all

================
Comment at: bolt/lib/Core/BinaryFunction.cpp:1054
+  (void)ReplaceSuccess;
+  assert(ReplaceSuccess && "failed to replace mem operand with symbol+off");
 }
----------------
maksfb wrote:
> maksfb wrote:
> > rafauler wrote:
> > > yota9 wrote:
> > > > Maybe change assert to if + exit ?
> > > Are we migrating away from assertions? This is not supposed to fail at all, unless we modify our codebase in weird ways, so I would like to assert here. But I'm not sure how hard are people trying to get rid of assertions.
> > > 
> > > Another reason this might fail is if somebody is writing a new backend and fails to replace the memory operand of a given instruction that they need to support replacing.
> > It it's a validation of internal assumptions, not an assumption about the input, then it should be an assertion.
> 
I don't follow this change, assertions (at least in this file) follow lower case messages with no period. Am I missing something?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134097/new/

https://reviews.llvm.org/D134097