[PATCH] D147697: [IR] Add TargetExtTypeClass

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/docs/LangRef.rst:3802-3803
+``zeroinitializer`` constant be valid. A complete list of type properties may be
+found in the documentation for ``llvm::TargetExtType::Property`` (`doxygen
+<https://llvm.org/doxygen/classllvm_1_1TargetExtType.html>`_).
+
----------------
I don't think the LangRef should refer to doxygen for things like this and should explicitly list the properties 


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:3259
+
+bool LLParser::parseTargetExtType(Type *&Result) {
+  auto Loc = Lex.getLoc();
----------------
Missing assembler tests for the various error cases 


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:3992
+  Vals.push_back(Fields.size());
+  for (const auto &Field : Fields) {
+    encodeSymbol(Vals, Field.first);
----------------
Use tuple unbinding syntax instead of .first/.second


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4009
+    } else {
+      llvm_unreachable("sdata value type not implemented");
+    }
----------------
Seems like it should be report_fatal_error


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147697