[PATCH] D89120: [lib/ObjectYAML] - Simplify the code that handles Content/Size fields.

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.

LGTM.



================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1303-1304
     ContiguousBlobAccumulator &CBA) {
-  if (Section.Content || Section.Size) {
-    SHeader.sh_size = writeContent(CBA, Section.Content, Section.Size);
+  if (!Section.Entries)
     return;
 
----------------
grimar wrote:
> jhenderson wrote:
> > I can see why this is needed, but why wasn't it needed before, and what's the impact of adding it? Same applies in a few other places too.
> Previously when we had either `Content` or `Size`, the code on the left wrote some data and returned.
> And we report an error when both `Entries` and `Content`/`Size` are used together much earlier, it is a normal situation.
> 
> After this patch we handle `Content/Size` for all types of sections in a single common place before `writeSectionContent` method is called.
> But we still have to call `writeSectionContent` to set section fields (e.g sh_entsize/sh_link etc) and/or
> handle section specific `Entries` (like here). Without this condition we just crash when trying to access an empty `Entries` in a loop below.
> 
> Perhaps worth to rename `writeSectionContent` to `finalizeSection` or alike.
Got it, thanks!


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

https://reviews.llvm.org/D89120