[PATCH] D78600: DenseStringElementsAttr added to default attribute types

[River Riddle via Phabricator via llvm-commits]

rriddle accepted this revision.
rriddle added a comment.
This revision is now accepted and ready to land.

Can you add support for SparseElementsAttr in a followup? It should be relatively straightforward as it uses DenseElementsAttr as the values internally.



================
Comment at: mlir/include/mlir/IR/Attributes.h:890
 
+  llvm::iterator_range<ElementIterator<StringRef>> getValues() const {
+    auto stringRefs = getStringRefs();
----------------
nit: drop the llvm:: from the iterator_range


================
Comment at: mlir/include/mlir/IR/Attributes.h:959
+  /// Return the raw StringRef data held by this attribute.
+  ArrayRef<StringRef> getStringRefs() const;
+
----------------
nit: getRawStringData?


================
Comment at: mlir/include/mlir/IR/Attributes.h:1006
+
+// An attribute class for representing dense arrays of strings. The structure
+// storing and querying a list of densely packed strings.
----------------
Please use /// for these comments.


================
Comment at: mlir/lib/IR/AttributeDetail.h:637
+
+    // Setup the ArrayRef
+    auto mutableCopy = MutableArrayRef<StringRef>(
----------------
nit: Please make the comment a complete sentence.


================
Comment at: mlir/lib/Parser/Parser.cpp:2243
+  case Token::string:
+    storage.emplace_back(/*isNegative*/ false, p.getToken());
+    p.consumeToken();
----------------
Please use the form /*...=*/


================
Comment at: mlir/lib/Parser/Parser.cpp:2301
   consumeToken(Token::kw_dense);
+
   if (parseToken(Token::less, "expected '<' after 'dense'"))
----------------
This seems unrelated now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78600