[PATCH] D39094: [ELF] - Cleanup of processSectionCommands().

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/OutputSections.cpp:119-120
   }
+  if (Noload)
+    Type = SHT_NOBITS;
 
----------------
grimar wrote:
> ruiu wrote:
> > This function is `addSection`, which is supposed to add a section to an output section, right? Adding this piece of code which is not related to adding sections to this function is not a good idea. This is how this function organically grown (and I took quite a long time to clean it up), so when you add code to some function, please stop and think if what you are about to do is semantically correct.
> This function not only adds section, but also initializes and changes the `Type`.
> And that happened before my change already, so I assume it is proper place to set `Type` because
> it probably desirable to change it in a single place of code, no ?
> 
> We can split this code (lines 97-120) to something like `setSectionType`, but that would be separate refactoring change.
> Will such change be OK for you ?
> 
George, I knew that was the reason why you added this piece of code to this function, but this is not right. If addSection mutates its class' Type based on a given argument, it is fine, but if it does something that is not related to a given section every time you pass a section to the function, something's not correct.


https://reviews.llvm.org/D39094