This is an archive of the discontinued LLVM Phabricator instance.

[XCOFF] [NFC] make StorageMappingClass/SymbolType member optional in MCSectionXCOFF class
ClosedPublic

Authored by shchenz on Feb 12 2021, 10:24 PM.

Details

Summary

This is a follow-up of D95931. In that patch, we made the function getXCOFFSection has optional parameters for StorageMappingClass and SymbolType.

This patch makes
1: the private class members MappingClass and Type optional;
2: add an override version constructor function that has no MappingClass and Type properties.

Non-csect sections like debug sections have no such properties.

Diff Detail

Event Timeline

shchenz created this revision.Feb 12 2021, 10:24 PM
shchenz requested review of this revision.Feb 12 2021, 10:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2021, 10:24 PM
shchenz edited the summary of this revision. (Show Details)Feb 12 2021, 10:27 PM
llvm/include/llvm/MC/MCSectionXCOFF.h
35–37

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).

45–47

I suggest leaving the this constructor alone and adding an override the omits the csect properties.

89

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.

92–96

Same comment about using an assert as above. Also, it would make sense to have an isCsect function.

llvm/lib/MC/XCOFFObjectWriter.cpp
608 ↗(On Diff #323521)

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.

894 ↗(On Diff #323521)

I think the pointer-like interface is less mental overhead when reading the code.

MaskRay added inline comments.
llvm/lib/MC/MCSectionXCOFF.cpp
42

assert is not needed because getValue() would fail too

llvm/lib/MC/MCSectionXCOFF.cpp
42

I envision this patch ending up with some asserts on isCsect because custom asserts are friendlier when they trigger.

shchenz updated this revision to Diff 323587.Feb 13 2021, 7:58 PM

1: make the get function as non Optional

shchenz updated this revision to Diff 323588.Feb 13 2021, 8:02 PM

1: add isCsect member function

Thanks for your review @hubert.reinterpretcast @MaskRay

Since now the get functions are not optional, so it should not impact the callers of them. But we need to revisit these functions when we add object output XCOFF DWARF support. I will leave the handlings in that follow up patch.

llvm/include/llvm/MC/MCSectionXCOFF.h
35–36

If IsCsect is true exactly when CsectProp is not None, then I suggest just using CsectProp and having isCsect() return static_cast<bool>(CsectProp).

shchenz edited the summary of this revision. (Show Details)Feb 13 2021, 8:17 PM
shchenz updated this revision to Diff 323592.Feb 13 2021, 8:25 PM

1: eliminates the redundant IsCsect member

llvm/include/llvm/MC/MCSectionXCOFF.h
35–36

Thanks, updated.

LGTM with minor comments.

llvm/include/llvm/MC/MCSectionXCOFF.h
57

Minor nit: Might as well use the locally-available variable without the extra abstraction.

68

Please insert an empty line before comments like this to clearly indicate the associate of the comment with the following line.

70

Same comment re: empty line.

85

Minor nit: isCsect() has a friendlier name.

93

Same comment as above.

llvm/lib/MC/MCSectionXCOFF.cpp
79

Same comment as above.

This revision is now accepted and ready to land.Feb 13 2021, 8:53 PM
shchenz marked 6 inline comments as done.Feb 15 2021, 7:40 AM

Thanks for your review @hubert.reinterpretcast