[PATCH] [inline-asm] Fix scope of assembly macros

Sergey Dmitrouk sdmitrouk at accesssoftek.com
Wed Dec 3 01:24:04 PST 2014


> I guess we need this if we want gcc compatibility.

Not sure if it's just about "gcc compatibility". C standard says:
```
The asm keyword may be used to insert assembly language directly into the translator output.
```
Assuming that there is one "translator output" per translation unit, the statement above
makes me think that there must be one common scope for all such directives.

Thanks for reviewing this.

================
Comment at: lib/MC/MCParser/AsmParser.cpp:55-56
@@ +54,4 @@
+/// string is independent from its creator.
+class MCAsmMacroArgToken
+{
+  /// Storage for token value.
----------------
rnk wrote:
> Please don't use Allman bracing, put the following open brace on the same line for ifs, fors, tag types, etc.
Right, I know how it should be formatted from the style guide, still did it wrong.

================
Comment at: lib/MC/MCParser/AsmParser.cpp:529
@@ -488,3 +528,3 @@
 
 AsmParser::AsmParser(SourceMgr &_SM, MCContext &_Ctx, MCStreamer &_Out,
                      const MCAsmInfo &_MAI)
----------------
rnk wrote:
> This constructor will be removed once clang is updated, right?
Correct.

================
Comment at: lib/MC/MCParser/AsmParser.cpp:546-552
@@ +545,9 @@
+{
+  CurBuffer = SrcMgr.getMainFileID();
+  MacrosEnabledFlag = true;
+  HadError = false;
+  CppHashLineNumber = 0;
+  AssemblerDialect = ~0U;
+  IsDarwin = false;
+  ParsingInlineAsm = false;
+
----------------
rnk wrote:
> Please leave these in initializer lists, even at the cost of duplication. The duplication will be gone once the alternate ctor is deleted.
OK.

================
Comment at: lib/MC/MCParser/AsmParser.cpp:1918
@@ -1858,1 +1917,3 @@
 
+MCAsmMacroArgToken::MCAsmMacroArgToken(const AsmToken &_Token) {
+  Value = _Token.getString();
----------------
rnk wrote:
> Underscores in variable names aren't really used in LLVM. Can you name this parameter something like TokenToCopy or CopiedToken?
I just copied style from "surrounding code" (see `AsmParser` constructors above), now renamed to `OriginalToken`.
Maybe I should make additional cleanup commit to remove leading underscores from `AsmParser`?

================
Comment at: lib/MC/MCParser/AsmParser.cpp:4803
@@ +4802,3 @@
+
+MCAsmContext::MCAsmContext()
+{
----------------
rnk wrote:
> More Allman
I just used output of `clang-format` as a guide, it found a couple more formatting issues in changed code.

http://reviews.llvm.org/D6383






More information about the llvm-commits mailing list