[PATCH] D45346: [LLVM-C] Audit Inline Assembly APIs for Consistency

whitequark via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 5 19:12:37 PDT 2018


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

Excellent work, thanks!

Regarding `LLVMSetModuleInlineAsm2`, I have two thoughts:

1. It is likely that actually passing a literal `\0` in inline assembly will confuse the assembler. In fact, I think both internal and external assemblers will refuse to process it.
2. Silently truncating on `\0` is likely to hide a bug elsewhere instead, so an explicit error is good.
3. Consistency with other APIs is good, and making the job of FFI bindings easier is also good (some of them autogenerate code from signatures).
4. Code churn is bad, and in this case even the above isn't a particularly strong justification for code churn.

As a result, I think we should merge this as-is, but do not take aggressive steps to remove `LLVMSetModuleInlineAsm`. It is enough that all new code will use `LLVMSetModuleInlineAsm2`.


https://reviews.llvm.org/D45346





More information about the llvm-commits mailing list