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

[Eric Schweitz via Phabricator via llvm-commits]

schweitz 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);
 
----------------
mehdi_amini wrote:
> 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?
Done


================
Comment at: flang/lib/Optimizer/Support/KindMapping.cpp:154
 
 static MatchResult parseTypeID(LLVMTypeID &result, const char *&ptr) {
   if (mlir::succeeded(matchString(ptr, "Half"))) {
----------------
mehdi_amini wrote:
> somehow unrelated to this patch, but the reference here is unusual, is this needed/useful?
It's required. This is the current cursor position walking through the string.


================
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();
----------------
mehdi_amini wrote:
> Seems like the constructor can fail here, should this be a factory function returning an optional then?
> Otherwise the caller can't really know...
Failure here is catastrophic. I'll change this to an error message with abort semantics.


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


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

https://reviews.llvm.org/D96362