rjmccall added inline comments.
================ Comment at: include/llvm/IR/IRBuilder.h:1138 + + return MetadataAsValue::get(Context, RoundingMDS); + } ---------------- 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. 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
