[PATCH] D102277: [llvm-objcopy][NFC] Refactor CopyConfig structure - categorize options.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.cpp:561
       const uint8_t Invalid = 0xff;
-      ResConfig.NewSymbolVisibility =
+      const_cast<ConfigManager *>(this)->ELF.NewSymbolVisibility =
           StringSwitch<uint8_t>(*NewSymbolVisibility)
----------------
jhenderson wrote:
> `const_cast` is always a bit scary to me. Would it make more sense to make `NewSymbolVisibility` and `SymbolsToAdd` mutable in the ELFConfig?
using mutable inside ELFConfig will allow to change the fields by consumer:


```
struct ELFConfig {
  mutable uint8_t NewSymbolVisibility;
};

void consumer (const ELFConfig& ELFConfig ) {
   ELFConfig.NewSymbolVisibility = STV_HIDDEN; // <<<<< incorrect usage
}
```

That would be not proper usage. The ConfigManager::getELFConfig() uses lazy initialization pattern. It is usually OK to use const_cast for lazy initialization. it is used in other places too:

llvm/CodeGen/ScheduleDAG.h


```
    unsigned getDepth() const {
      if (!isDepthCurrent)
        const_cast<SUnit *>(this)->ComputeDepth();
      return Depth;
    }
```

So I think it is better to not use mutable inside ELFConfig.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102277