[PATCH] D67529: [TextAPI] Introduce TBDv4

[Juergen Ributzka via Phabricator via llvm-commits]

ributzka added inline comments.


================
Comment at: llvm/lib/TextAPI/MachO/TextStub.cpp:352
+
+  static void mapping(IO &IO, MetadataSection &Section, unsigned &Ctx) {
+    IO.mapRequired("targets", Section.Targets);
----------------
I don't think the context has to be an unsigned. Now that you have an enum, you can use that instead. Please also update the coder further down that sets the context.


================
Comment at: llvm/lib/TextAPI/MachO/TextStub.cpp:355
+    switch (Ctx) {
+    default:
+      return;
----------------
The switch statement could be improved by removing the default. That way the compiler will emit a warning if the switch statement isn't fully covered. There should also be a llvm_unreachable after the switch statement to catch unexpected values.


================
Comment at: llvm/lib/TextAPI/MachO/TextStub.cpp:779
         llvm_unreachable("unexpected file type");
-      case FileType::TBD_V1:
-        // Don't write the tag into the .tbd file for TBD v1.
-        break;
+      case FileType::TBD_V4:
+        mappingTBD_V4(IO, File);
----------------
nitpick: the order it quite random now.


================
Comment at: llvm/lib/TextAPI/MachO/TextStub.cpp:794
 
+    MappingNormalization<NormalizedTBD, const InterfaceFile *> Keys(IO, File);
     IO.mapRequired("archs", Keys->Architectures);
----------------
Factoring this out into a separate function would make this code easier to understand, because we fall-through to this code, but call the mappingTBD_V4 method in the other case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67529