kpn marked an inline comment as done. kpn added inline comments.
================ Comment at: include/llvm/IR/IRBuilder.h:1138 + + return MetadataAsValue::get(Context, RoundingMDS); + } ---------------- rjmccall wrote: > kpn wrote: > > rjmccall wrote: > > > Huh? You build an `MDNode` that wraps an `MDString` and then immediately > > > extract the `MDString` from it and drop the `MDNode`? > > > > > > I think you should just have a function somewhere that returns the > > > correct metadata string for a `ConstrainedRoundingKind`, and then the > > > code here is much more obvious. Maybe this can be wherever you declare > > > the enums. You can also have a function that goes the other way and > > > returns an `Optional<ConstrainedRoundingKind>`. > > > > > > Function name should include `FP`. > > > > > > Same points apply to the next function. > > Well, when you put it that way, your proposed change does result in much > > simpler code. > > > > The resulting code isn't that much more than the switch. I hope it is now > > easy enough to read. These functions are private to the class and just > > exist to be helpers. I don't see a need to put the switch table into a > > different function, but I'm willing to be convinced. > I feel like having the enum->string mapping available somewhere independent > of `IRBuilder` will probably be useful to someone, and it nicely separates > concerns in any case . You could just make it a static method on > `ConstrainedFPIntrinsic` along with the reverse operation. Ok, will do. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53157/new/ https://reviews.llvm.org/D53157 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
