This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Summary

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

llvm/include/llvm/BinaryFormat/XCOFF.h
409

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

410

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.

411–414

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

llvm/include/llvm/MC/MCContext.h
573–574

Use llvm::Optional.

llvm/lib/MC/MCContext.cpp
675

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.

llvm/include/llvm/BinaryFormat/XCOFF.h
410

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.

llvm/lib/MC/MCContext.cpp
675

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.

llvm/lib/MC/MCContext.cpp
675

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