[PATCH] D53945: [TextAPI] TBD Reader/Writer

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

Nitpicks aside, the mach-o side of this looks good to me. :)

Is there any other outstanding feedback?



================
Comment at: llvm/include/llvm/TextAPI/InterfaceFile.h:165
+  ///
+  /// This is used by the YAML writter to identify the specification it should.
+  ///
----------------
Looks like a partial comment.

And "writter" should be "writer".


================
Comment at: llvm/include/llvm/TextAPI/InterfaceFile.h:181-184
+  /// Specify the set of supported architectures by this file.
+  void setArchitectures(ArchitectureSet Architectures_) {
+    Architectures |= Architectures_;
+  }
----------------
Should this be called "addArchitectures" instead, given that it's an additive operation?


================
Comment at: llvm/lib/TextAPI/TextStub.cpp:183-191
+inline Flags operator|(const Flags a, const Flags b) {
+  return static_cast<Flags>(static_cast<unsigned>(a) |
+                            static_cast<unsigned>(b));
+}
+
+inline Flags operator|=(Flags &a, const Flags b) {
+  a = static_cast<Flags>(static_cast<unsigned>(a) | static_cast<unsigned>(b));
----------------
You might be able to use llvm/ADT/BitmaskEnum.h here, rather than defining these operators yourself.


================
Comment at: llvm/lib/TextAPI/TextStub.cpp:394-395
+        std::sort(Section.Symbols.begin(), Section.Symbols.end());
+        std::sort(Section.Classes.begin(), Section.Classes.begin());
+        std::sort(Section.ClassEHs.begin(), Section.ClassEHs.begin());
+        std::sort(Section.IVars.begin(), Section.IVars.end());
----------------
These sorts are no-ops, along the same lines as the ones Duncan pointed out above.


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

https://reviews.llvm.org/D53945