[PATCH] D88288: [Archive] Don't throw away errors for malformed archive members

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht accepted this revision.
rupprecht added a comment.
This revision is now accepted and ready to land.

There's a build warning that needs to be fixed, but otherwise LG.



================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:499-501
+          getSymbols(Buf, SymNames, HasObject);
+      if (auto E = SymbolsOrErr.takeError())
+        return std::move(E);
----------------
nit: looks like tab characters were inserted, can you run clang-format?


================
Comment at: llvm/lib/Object/SymbolicFile.cpp:47
+
   switch (Type) {
   case file_magic::bitcode:
----------------
Splitting up the switch leads to `-Wswitch` errors because now each switch statement is incomplete:

```
/home/rupprecht/src/llvm-project/llvm/lib/Object/SymbolicFile.cpp:47:11: warning: 8 enumeration values not handled in switch: 'unknown', 'archive', 'macho_universal_binary'... [-Wswitch]
  switch (Type) {
          ^
/home/rupprecht/src/llvm-project/llvm/lib/Object/SymbolicFile.cpp:97:11: warning: 23 enumeration values not handled in switch: 'bitcode', 'elf', 'elf_relocatable'... [-Wswitch]
  switch (Type) {
          ^
```

Normally the right fix is to add a default switch case to handle all the unused cases. If you're concerned about the list of `file_magic::unknown, file_magic::archive, ...` in each method going out of sync in the two definitions (and new enums accidentally being handled with the default case), you could define that list in a macro, e.g.

```
#define NOT_SYMBOLIC_FILES \
  case file_magic::unknown: \
  case file_magic::archive: \
  case file_magic::coff_cl_gl_object: \
...
  case file_magic::tapi_file

  switch (Type) {
    NOT_SYMBOLIC_FILES:
      return false;
    default:
      return true;
  }
```

(note no trailing `:` in the last case you can write `NOT_SYMBOLIC_FILES:` and not confused IDEs)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88288