[PATCH] D96641: [XCOFF] [NFC] make StorageMappingClass/SymbolType member optional in MCSectionXCOFF class

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/MC/MCSectionXCOFF.h:35-36
 
-  XCOFF::StorageMappingClass MappingClass;
-  XCOFF::SymbolType Type;
+  Optional<XCOFF::StorageMappingClass> MappingClass;
+  Optional<XCOFF::SymbolType> Type;
   MCSymbolXCOFF *const QualName;
----------------
I would suggest storing a `XCOFF::CsectProperties` member. That semantically reflects that these are either both present or both non-present (which is the case at the level of the object file encoding as well).


================
Comment at: llvm/include/llvm/MC/MCSectionXCOFF.h:42-43
 
-  MCSectionXCOFF(StringRef Name, XCOFF::StorageMappingClass SMC,
-                 XCOFF::SymbolType ST, SectionKind K, MCSymbolXCOFF *QualName,
-                 MCSymbol *Begin, StringRef SymbolTableName,
-                 bool MultiSymbolsAllowed)
+  MCSectionXCOFF(StringRef Name, Optional<XCOFF::StorageMappingClass> SMC,
+                 Optional<XCOFF::SymbolType> ST, SectionKind K,
+                 MCSymbolXCOFF *QualName, MCSymbol *Begin,
----------------
I suggest leaving the this constructor alone and adding an override the omits the csect properties.


================
Comment at: llvm/include/llvm/MC/MCSectionXCOFF.h:70
 
-  XCOFF::StorageMappingClass getMappingClass() const { return MappingClass; }
-  XCOFF::StorageClass getStorageClass() const {
+  Optional<XCOFF::StorageMappingClass> getMappingClass() const {
+    return MappingClass;
----------------
It seems (from the code later in this patch) that the return type of this doesn't need to be `Optional`; instead, this function should `assert` that the current object here represents a csect.


================
Comment at: llvm/include/llvm/MC/MCSectionXCOFF.h:76
   }
-  XCOFF::SymbolType getCSectType() const { return Type; }
+  Optional<XCOFF::SymbolType> getCSectType() const { return Type; }
   MCSymbolXCOFF *getQualNameSymbol() const { return QualName; }
----------------
Same comment about using an `assert` as above. Also, it would make sense to have an `isCsect` function.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:608
 
 void XCOFFObjectWriter::writeSymbolTableEntryForControlSection(
     const ControlSection &CSectionRef, int16_t SectionIndex,
----------------
The function is already named as being csect-specific. I suggest leaving this function signature alone and adding a new function interfaces for the specific non-csect cases.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:894
+  assert(Sec->getCSectType() && "Not a csect!");
+  return EncodedAlign | Sec->getCSectType().getValue();
 }
----------------
I think the pointer-like interface is less mental overhead when reading the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96641