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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast accepted this revision.
hubert.reinterpretcast added a comment.
This revision is now accepted and ready to land.

LGTM with minor comments.



================
Comment at: llvm/include/llvm/MC/MCSectionXCOFF.h:56
     // A csect is 4 byte aligned by default, except for undefined symbol csects.
-    if (Type != XCOFF::XTY_ER)
+    if (CsectProp->Type != XCOFF::XTY_ER)
       setAlignment(Align(DefaultAlignVal));
----------------
Minor nit: Might as well use the locally-available variable without the extra abstraction.


================
Comment at: llvm/include/llvm/MC/MCSectionXCOFF.h:67
+    assert(QualName != nullptr && "QualName is needed.");
+    // FIXME: use a more meaningful name for non csect sections.
+    QualName->setRepresentedCsect(this);
----------------
Please insert an empty line before comments like this to clearly indicate the associate of the comment with the following line.


================
Comment at: llvm/include/llvm/MC/MCSectionXCOFF.h:69
+    QualName->setRepresentedCsect(this);
+    // Set default alignment 4 for all non csect sections for now.
+    // FIXME: set different alignments according to section types.
----------------
Same comment re: empty line.


================
Comment at: llvm/include/llvm/MC/MCSectionXCOFF.h:84
+  XCOFF::StorageMappingClass getMappingClass() const {
+    assert(CsectProp.hasValue() &&
+           "Only csect section has mapping class property!");
----------------
Minor nit: `isCsect()` has a friendlier name.


================
Comment at: llvm/include/llvm/MC/MCSectionXCOFF.h:92
+  XCOFF::SymbolType getCSectType() const {
+    assert(CsectProp.hasValue() &&
+           "Only csect section has symbol type property!");
----------------
Same comment as above.


================
Comment at: llvm/lib/MC/MCSectionXCOFF.cpp:79
+bool MCSectionXCOFF::isVirtualSection() const {
+  assert(CsectProp.hasValue() && "Only csect section can be virtual!");
+  return XCOFF::XTY_CM == CsectProp->Type;
----------------
Same comment as above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96641