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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:409
 
+typedef struct CsectProperty {
+  CsectProperty(uint8_t VSMC, uint8_t VST) : SMC(VSMC), ST(VST) {}
----------------
There's no need to use the C-style typedef + tag type pattern in C++ code. Also, the name should reflect that this type stores csect properties (and is not a selector between csect properties).


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:410
+typedef struct CsectProperty {
+  CsectProperty(uint8_t VSMC, uint8_t VST) : SMC(VSMC), ST(VST) {}
+  // StorageMappingClass
----------------
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.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:411-414
+  // StorageMappingClass
+  uint8_t SMC;
+  // SymbolType
+  uint8_t ST;
----------------
I suggest keeping the very short names for block-scope variables only.


================
Comment at: llvm/include/llvm/MC/MCContext.h:574-575
+    MCSectionXCOFF *getXCOFFSection(StringRef Section, SectionKind K,
+                                    const XCOFF::CsectProperty &CsectProp =
+                                        XCOFF::CsectProperty(0xFF, 0xFF),
                                     bool MultiSymbolsAllowed = false,
----------------
Use `llvm::Optional`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95931