[PATCH] D62362: [COFF] De-virtualize Chunk and SectionChunk

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked an inline comment as done.
rnk added inline comments.


================
Comment at: lld/COFF/Chunks.h:377
+
+inline bool Chunk::isHotPatchable() const {
+  if (isa<SectionChunk>(this))
----------------
rnk wrote:
> aganea wrote:
> > Since you're doing this, I'm just wondering if the behavior for `isHotPatchable` shouldn't all be here? I find there's value having all related code together. Maybe there are other cases for the other "virtual-ish" functions below.
> > ```
> > inline bool Chunk::isHotPatchable() const {
> >   if (ChunkKind == SectionKind)
> >     return File->HotPatchable;
> >   return ChunkKind  == ImportThunk; // add ImportThunk kind
> > }
> > ```
> > 
> > 
> I think the way to go is to make this change, and then tackle the remaining virtual methods one by one. This patch replaces the `hasData()` virtual method with the `HasData` bit, and there are a number of other methods here that could probably use the same treatment.
> 
> Initially I tried to lift section Characteristics up into Chunk in the first version of this patch, but it didn't work out, and I backed it out. I put the P2Align data in to Characteristics and had all the Chunk subtypes simply set their Characteristics in the constructor, but I needed another byte for the ChunkKind, or at least a bit to indicate SectionChunk vs. NonSectionChunk. I couldn't get the data packed in right without doing more unrelated rearrangements, so I left it for later.
This suggested change became D62422.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D62362