[PATCH] D95931: [XCOFF] [NFC] make csect properties optional when create a new section

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

@hubert.reinterpretcast Thanks for your comments. Updated accordingly.



================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:410
+typedef struct CsectProperty {
+  CsectProperty(uint8_t VSMC, uint8_t VST) : SMC(VSMC), ST(VST) {}
+  // StorageMappingClass
----------------
hubert.reinterpretcast wrote:
> Because we're using `llvm::Optional`, we can use the enumeration types.
> 
> Moot point: Is attaching "V" in front a common pattern in LLVM? The way the name lookup works means that using the same name for the constructor parameter and the class member is fine.
Very good idea. Before using `Optional`, I have to use `0xff` as the default value for `StorageMappingClass` and `SymbolType`, so I need a `unsigned` type for the parameter. Now I can use the enumeration type because the default value for an optional parameter can be `None`.

> Is attaching "V" in front a common pattern in LLVM

Seems this is not common use. I added that for differentiating the class member name and function parameter name. But you are right, we are ok here to use the same name for them.


================
Comment at: llvm/lib/MC/MCContext.cpp:674
+  auto IterBool = XCOFFUniquingMap.insert(std::make_pair(
+      XCOFFSectionKey{Section.str(), (XCOFF::StorageMappingClass)CsectProp.SMC},
+      nullptr));
----------------
hubert.reinterpretcast wrote:
> This is eventually going to need to handle the case where the section is not a csect, but for this patch, I think it is okay to write it as it all XCOFF sections are csects. The patch that introduces non-csect XCOFF sections will need to do the update to this.
Updated in D95518 and I added a new NFC patch D96641 to make class member also optional


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95931