[PATCH] D73047: [ELF] Refactor uses of getInputSections to improve efficiency NFC

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/ELF/OutputSections.cpp:469
+namespace {
+void forEachInputSectionDescription(
+    const OutputSection *os, function_ref<bool(InputSectionDescription *)> f) {
----------------
peter.smith wrote:
> It would definitely be worth adding a comment, explaining that f must return true to continue and false to early terminate. I didn't find it obvious until reading the loop body and the two examples.
> 
> I thought about trying a different name (there is an identically named helper in Relocations.cpp that does something similar) but I couldn't come up with anything succinct. The best I could come up with was forEachInputSectionDescriptionUntilFalse().
`StringRef` has `take_while` and `take_until`
It could be `forEachInputSectionDescriptionDoWhile` probably.
But see below.


================
Comment at: lld/ELF/OutputSections.cpp:478
+
+InputSection *getFirstInputSection(const OutputSection *os) {
+  InputSection *ret = nullptr;
----------------
peter.smith wrote:
> Given that we only have 2 clients of forEachInputSectionDescription and no nested loops , it might be worth having two simpler functions that have their own loop
> ```
> InputSection *getFirstInputSection(const OutputSection *os) {
>   for (BaseCommand *base : os->sectionCommands)
>     if (auto *isd = dyn_cast<InputSectionDescription>(base))
>       if (!isd->sections.empty())
>         return isd->sections.front();
>   return nullptr;
> }
> ```
> getInputSections can remain the same as before.
> 
I agree. No need to introduce `forEachInputSectionDescription` probably.


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

https://reviews.llvm.org/D73047