[PATCH] D113934: [llvm] adapt DWARFExpression.h for 6b9b86db9dd974585a5c71cf2e5231d1e3385f7c

[Krasimir Georgiev via Phabricator via llvm-commits]

krasimir added a comment.

Sorry for submitting quickly, I just wanted to unblock build bots. I still think that this patch is OK considering the weirdness of this Operation class.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h:110
     uint64_t Offset;
-    Operation Op;
+    mutable Operation Op;
     iterator(const DWARFExpression *Expr, uint64_t Offset)
----------------
dexonsmith wrote:
> Do callers really want/need to modify the `Operation`?
I think yes, 


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h:110
     uint64_t Offset;
-    Operation Op;
+    mutable Operation Op;
     iterator(const DWARFExpression *Expr, uint64_t Offset)
----------------
krasimir wrote:
> dexonsmith wrote:
> > Do callers really want/need to modify the `Operation`?
> I think yes, 
I believe yes: none of the public member functions is const
https://github.com/llvm/llvm-project/blob/0b17336f793108a7b10c3fa913039144ef1d0f61/llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h#L89.
Some of the methods of Operation can be made const, but ultimately it's pretty mutable. For example, Operation::verify() sets Error (https://github.com/llvm/llvm-project/blob/fcd07f810781c1754c2e7e9641fd18b95fa1368c/llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp#L376) and is used in a loop in `DWARFExpression::verify`: https://github.com/llvm/llvm-project/blob/fcd07f810781c1754c2e7e9641fd18b95fa1368c/llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp#L387.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h:127
 
-    class Operation &operator*() {
+    class Operation &operator*() const {
       return Op;
----------------
dexonsmith wrote:
> Wondering if this should be `const class Operation &operator*`.
ditto, the problem is that all public members are non-const, so even checking for stuff needs non-const ref.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113934