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

Mon Oct 10 23:14:50 PDT 2022

maksfb added inline comments.

================
Comment at: bolt/lib/Core/BinaryFunction.cpp:1054
+  (void)ReplaceSuccess;
+  assert(ReplaceSuccess && "failed to replace mem operand with symbol+off");
 }
----------------
rafauler wrote:
> 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?
You are right about the existing assertions. The idea is to move toward LLVM standard in messages and assertions, hence the suggestion.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134097