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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I haven't looked at all of this yet, by a long way, but here are a few comments on the first few files.



================
Comment at: llvm/include/llvm/TextAPI/ArchitectureSet.h:1
+//===- llvm/TextAPI/ArchitectureSet.h - ArchitectureSet ---------*- C++ -*-===//
+//
----------------
There's a lot of code in this header that feels like it belongs in a separate .cpp. Doing so would make the class API easier to read, and might reduce the include dependencies.


================
Comment at: llvm/include/llvm/TextAPI/ArchitectureSet.h:22
+#include <limits>
+#include <stddef.h>
+#include <tuple>
----------------
I'd be surprised if you need to explicitly include this header, but if you do, it should be cstddef probably.


================
Comment at: llvm/include/llvm/TextAPI/ArchitectureSet.h:40
+  ArchitectureSet(Architecture Arch) : ArchitectureSet() { set(Arch); }
+  ArchitectureSet(const std::vector<Architecture> &Archs) : ArchitectureSet() {
+    for (auto Arch : Archs) {
----------------
This should probably use an `ArrayRef`.

Do you need to explicit call the `ArchitectureSet` default constructor in these two?


https://reviews.llvm.org/D53945