inline-asm: Fix some warnings #3393
Conversation
|
@badumbatish I'm going to make some changes to inline assembly, if you have time to review some PRs I'd appreciate your input :) I'll ping you |
Happy to help :) i'll try to put in some reviews soon! |
CohenArthur
force-pushed
the
inline-asm-warnings
branch
from
2de537c to
63730a7
Compare
| @@ -62,6 +62,8 @@ const BiMap<std::string, BuiltinMacro> MacroBuiltin::builtins = {{ | |||
| {"concat_idents", BuiltinMacro::ConcatIdents}, | |||
| {"module_path", BuiltinMacro::ModulePath}, | |||
| {"asm", BuiltinMacro::Asm}, | |||
| // FIXME: Is that okay | |||
| {"llvm_asm", BuiltinMacro::Asm}, | |||
There was a problem hiding this comment.
I remembered llvm_asm was removed last summer since the llvm_asm! is not supported anymore. Has it changed?
There was a problem hiding this comment.
So it has been removed in recent Rust versions, but is still used in Rust 1.49 - so we'll need to support some form of it. I think the only thing we really need to support is llvm_asm! in core, and I think we can use regular inline assembly for this
There was a problem hiding this comment.
It looks like the only llvm_asm! usages in the rustc libraries are for black_box, which seems fairly simple, and a few stdarch things.
| @@ -39,13 +39,18 @@ std::map<AST::InlineAsmOption, std::string> InlineAsmOptionMap{ | |||
| std::set<std::string> potentially_nonpromoted_keywords | |||
| = {"in", "out", "lateout", "inout", "inlateout", "const", "sym", "label"}; | |||
|
|
|||
| // Helper function strips the beginning and ending double quotes from a | |||
| // string. | |||
| std::string | |||
| strip_double_quotes (const std::string &str) | |||
| { | |||
There was a problem hiding this comment.
it's probably just a nitpick on myself but back then it didnt occur to me that somebody might use this on sth like str = "'dsda'" or "abcd" despite the name being strip_double_quotes().
I think an assertion on the first and last character being double quotes would be nice :)
There was a problem hiding this comment.
Fixed in latest version
CohenArthur
force-pushed
the
inline-asm-warnings
branch
from
63730a7 to
d096c74
Compare
gcc/rust/ChangeLog:
* expand/rust-macro-builtins-asm.cc (strip_double_quotes): Special case empty
strings ("\"\"").
(parse_reg_operand): Remove use of the `struct` keyword.
(parse_reg_operand_in): Likewise.
(parse_reg_operand_out): Likewise.
* expand/rust-macro-builtins.cc: Add llvm_asm! built-in macro as an alias to asm!.
CohenArthur
force-pushed
the
inline-asm-warnings
branch
from
d096c74 to
58fa050
Compare
|
LGTM :) |
| // we have to special case empty strings which just contain a set of quotes | ||
| // so, if the string is "\"\"", just return "" | ||
| if (result.size () == 2) | ||
| return ""; |
There was a problem hiding this comment.
Why is this special cased, anyways?
There was a problem hiding this comment.
Might be a good idea to use number 4 from https://en.cppreference.com/w/cpp/string/basic_string/basic_string
There was a problem hiding this comment.
because of the assertion later on, which I did not want to tweak. since this is a static function for that one specific file I was hoping to keep it as simple and as tailored as possible - it doesn't leak anywhere else and doesn't need to be too complicated
No description provided.