[PATCH] D57324: lld/coff: Implement some support for the comdat selection field

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/COFF/InputFiles.cpp:447
+  // Handle comdat leader.
   if (const coff_aux_section_definition *Def = ComdatDefs[SectionNumber]) {
     ComdatDefs[SectionNumber] = nullptr;
----------------
thakis wrote:
> This function is getting fairly long; probably want to move the whole contents of this if to its own function or something. But I thought I'd see if you have other comments first since moving the contents is easy to do.
I'm not too worried by the length of this function as long as each `if` ends with `return`. If each `if` ends with `return`, this function is virtually just a large `switch`, and a logic for each `case` is independent to another. That said I think it is perhaps better to move this function to a new function though.


================
Comment at: lld/COFF/InputFiles.cpp:461-464
+    if (Def->Selection >= (int)IMAGE_COMDAT_SELECT_NODUPLICATES &&
+        // Intentionally adds at IMAGE_COMDAT_SELECT_LARGEST: link.exe
+        // doesn't understand IMAGE_COMDAT_SELECT_NEWEST either.
+        Def->Selection <= (int)IMAGE_COMDAT_SELECT_LARGEST) {
----------------
I'd invert the condition and call `fatal` early to reduce the indentation depth of the regular code path.


================
Comment at: lld/COFF/InputFiles.cpp:508
+          error(Msg);
+      }
+
----------------
Is it safe to fall through? Maybe you want to do `continue` to skip it.


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

https://reviews.llvm.org/D57324