This is an archive of the discontinued LLVM Phabricator instance.

[XCOFF] [NFC] make csect properties optional for function getXCOFFSection

Authored by shchenz on Feb 3 2021, 1:48 AM.



This is from the review of D95518.
Per @hubert.reinterpretcast comments, we should make csect properties optional as they are not used by newly introduced debug sections in D95518.

Diff Detail

Event Timeline

shchenz created this revision.Feb 3 2021, 1:48 AM
shchenz requested review of this revision.Feb 3 2021, 1:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2021, 1:48 AM

gentle ping


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


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.


I suggest keeping the very short names for block-scope variables only.


Use llvm::Optional.


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.

shchenz updated this revision to Diff 323523.Feb 12 2021, 10:30 PM
shchenz marked 3 inline comments as done.

1: use llvm::Optional

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


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.


Updated in D95518 and I added a new NFC patch D96641 to make class member also optional

shchenz retitled this revision from [XCOFF] [NFC] make csect properties optional when create a new section to [XCOFF] [NFC] make csect properties optional for function getXCOFFSection.Feb 12 2021, 10:47 PM

LGTM with minor comment.


Thanks. Let's have a TODO comment about this function's need for revision to handle non-csects in the code anyway (so this patch is better self-contained).

This revision is now accepted and ready to land.Feb 13 2021, 2:21 PM
shchenz updated this revision to Diff 323585.Feb 13 2021, 7:55 PM

1: add the FIXME for None CsectProp