Page MenuHomePhabricator

[NFC][XCOFF] Using crtp to refactor the xcoff section header
ClosedPublic

Authored by DiggerLin on Thu, Oct 17, 12:42 PM.

Details

Summary

According to https://reviews.llvm.org/D68575#inline-617586, I create a NFC patch for it.

  1. Using crtp to refactor the xcoff section header
  2. Move the define of SectionFlagsReservedMask and SectionFlagsTypeMask from XCOFFDumper.cpp to XCOFFObjectFile.h

Diff Detail

Event Timeline

DiggerLin created this revision.Thu, Oct 17, 12:42 PM
jasonliu added a comment.EditedWed, Oct 23, 12:29 PM

General comments about the direction of this patch:
Previous comments(https://reviews.llvm.org/D68575#inline-617586) suggests to put those two constant value into XCOFF.h.
I guess the reason for that is we want all the other component of in llvm to be able to use that those two constant value. For example, XCOFFObjectWriter.
But this patch currently put those two values in XCOFFObjectFile.h. The headers in llvm/Object seems to be strictly used only by all the llvm tooling that has an interest in the object itself, but not used by the llvm backend at all.

I think the direction suggested in D68575 makes more sense, as those two constant values are more related to object format, and we would want to use them in more than llvm toolings.

DiggerLin added a comment.EditedThu, Oct 24, 11:51 AM

General comments about the direction of this patch:
Previous comments(https://reviews.llvm.org/D68575#inline-617586) suggests to put those two constant value into XCOFF.h.
I guess the reason for that is we want all the other component of in llvm to be able to use that those two constant value. For example, XCOFFObjectWriter.
But this patch currently put those two values in XCOFFObjectFile.h. The headers in llvm/Object seems to be strictly used only by all the llvm tooling that has an interest in the object itself, but not used by the llvm backend at all.

I think the direction suggested in D68575 makes more sense, as those two constant values are more related to object format, and we would want to use them in more than llvm toolings.

if both xcoffobjectfilewriter.cpp and xcoffobjectfile.cpp use these two constant, I agreed to put them in the xcoff.h but I do not think we will use them in the xcoffobjectfilewriter.cpp(that is means they only use in xcoff decode tools, if only use in the xcoff decode tool, it is better to put them in the xcoffobjectfile.h).

LGTM with minor comments.

llvm/include/llvm/Object/XCOFFObjectFile.h
50–63

Typo: s/Setion/Section/;
Since the base class does not represent a section header by itself, name this something like XCOFFSectionHeaderBase.

llvm/lib/Object/XCOFFObjectFile.cpp
49

s/Explict/Explicitly/;
s/class/classes/;

54

Typo: s/Setion/Section/;

59

Same comment.

This revision is now accepted and ready to land.Wed, Oct 30, 1:01 PM
This revision was automatically updated to reflect the committed changes.