[PATCH] D96362: [flang][fir] Update the kind mapping class.

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: flang/include/flang/Optimizer/Support/KindMapping.h:58
+  explicit KindMapping(mlir::MLIRContext *context, llvm::StringRef map,
+                       llvm::ArrayRef<KindTy> defs = llvm::None);
 
----------------
There is an implicit size and ordering expectation for defs that seems quite subtle and deserve documentation here.

I also don't see a test which exercise this extra parameter?


================
Comment at: flang/lib/Optimizer/Support/KindMapping.cpp:154
 
 static MatchResult parseTypeID(LLVMTypeID &result, const char *&ptr) {
   if (mlir::succeeded(matchString(ptr, "Half"))) {
----------------
somehow unrelated to this patch, but the reference here is unusual, is this needed/useful?


================
Comment at: flang/lib/Optimizer/Support/KindMapping.cpp:194
   if (mlir::failed(parse(map))) {
+    mlir::emitError(mlir::UnknownLoc::get(context), "could not parse kind map");
     intMap.clear();
----------------
Seems like the constructor can fail here, should this be a factory function returning an optional then?
Otherwise the caller can't really know...


================
Comment at: flang/lib/Optimizer/Support/KindMapping.cpp:273
+fir::KindMapping::setDefaultKinds(llvm::ArrayRef<KindTy> defs) {
+  if (defs.size() == 0) {
+    // generic front-end defaults
----------------
Nit: `if(defs.empty())`


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

https://reviews.llvm.org/D96362