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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

General requests

1. Can we namespace MachO specific stuff? I'm fine airing on the side of putting things in the MachO namespace and dedupping later if possible.
2. This is pretty big. Can we break this up at all?
3. I've thus far ignored the parts that are MachO specific and assumed someone else more qualified would handle that. Before giving an LGTM I'd like to see review of that code. Breaking this up would help me to do that if a person more expierenced with MachO isn't present to do that.



================
Comment at: llvm/include/llvm/TextAPI/Registry.h:37
+  virtual ~Reader();
+  virtual bool canRead(file_magic Magic, MemoryBufferRef BufferRef,
+                       const Registry &) const = 0;
----------------
Where is file_magic defined? I'm going to make a bunch of, very possibly wrong, assumptions here.

Presumably it's computed from MemoryBufferRef as well so requiring the user to pre-compute it is a bit odd. Also not all conceivable formats have magic.


================
Comment at: llvm/include/llvm/TextAPI/Registry.h:48
+  virtual ~Writer();
+  virtual bool canWrite(const InterfaceFile *File, const Registry &) const = 0;
+  virtual Error writeFile(raw_ostream &OS, const InterfaceFile *File,
----------------
What would be a case where this would return false?


================
Comment at: llvm/include/llvm/TextAPI/Registry.h:53
+
+class YAMLDocumentHandler {
+public:
----------------
Can you explain why this is needed?  It seems like this could be split up into a Reader and a Writer.


================
Comment at: llvm/lib/TextAPI/TextStub.cpp:591
+  auto Str = BufferRef.getBuffer().trim();
+  if (!(Str.startswith("---\narchs:") ||
+        Str.startswith("--- !tapi-tbd-v1\n")) ||
----------------
An issue Armando hit was that this doesn't allow comments at the start of TAPI  files. It also just seems like a generally error prone way to check for the validity of such files IMO. I think a better way would be to just attempt to parse and handle the failure by attempting to parse the next format. It's unlikely that the parser would actually get very far before failing.


https://reviews.llvm.org/D53945