[cfe-commits] r161703 - in /cfe/trunk: include/clang/AST/Stmt.h lib/AST/Stmt.cpp

[Enea Zaffanella]

On 08/10/2012 11:36 PM, Chad Rosier wrote:
> Author: mcrosier
> Date: Fri Aug 10 16:36:25 2012
> New Revision: 161703
>
> URL: http://llvm.org/viewvc/llvm-project?rev=161703&view=rev
> Log:
> [ms-inline asm] Avoid extra allocations by making this an array of StringRefs.
[...]


Hello.

Since you are working on the ms-inline asm statements,
we would like to propose a change to the AST representation.

Currently, consecutive MS asm statements are merged into a single 
MSAsmStmt node.

This choice negatively affects source code fidelity.
For instance, there is a single source location for just the first of 
the possibly many consecutive __asm keywords and there is no faithful 
tracking of braces.
Moreover, it seems to also complicate later code trying to reconstruct 
the spacing and line-breaking around the asm tokens (namely, the use of 
array LineEnds and of static function needSpaceAsmToken).

As things are now, when parsing the following:

   __asm {
     mov  eax, dword ptr [0]
     mov  edx, dword ptr [4]
   }

we obtain the string
   "mov eax, dword ptr[0]mov edx, dword ptr[4]"
with no line break.


Wouldn't thing be more simple if each __asm keyword is mapped onto its 
own MSAsmStmt node?
(Side note: afawct, this is what happens for non-MS AsmStmt nodes.)

 From the source code fidelity perspective, we would get a source 
location for each __asm keyword and we will be able to properly tell if 
it is using braces or not.
Moreover, the logic for producing the asm string would be simpler too: 
there will be no need to use an array LineEnds, because you can query 
each asm token with isAtStartOfLine().
As a matter of fact, token method hasLeadingSpace() could replace static 
function needSpaceAsmToken() to add a white space if there was one in 
the original sources.

For the sample above, we would thus obtain the string
   "mov eax, dword ptr [0]\nmov edx, dword ptr [4]"
with both the line break and the extra space before the "[".

Opinions?
Enea.