[cfe-commits] r44266 - in /cfe/trunk: AST/Stmt.cpp AST/StmtPrinter.cpp AST/StmtSerialization.cpp Parse/ParseStmt.cpp Sema/Sema.h Sema/SemaStmt.cpp include/clang/AST/Stmt.h include/clang/Parse/Action.h include/clang/Parse/Parser.h

[Ted Kremenek]

Anders,

The serialization code looks good to me.  A few comments (inline):

On Nov 21, 2007, at 5:36 PM, Anders Carlsson wrote:

> Modified: cfe/trunk/AST/StmtSerialization.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/AST/StmtSerialization.cpp?rev=44266&r1=44265&r2=44266&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/AST/StmtSerialization.cpp (original)
> +++ cfe/trunk/AST/StmtSerialization.cpp Wed Nov 21 19:36:19 2007
> @@ -197,8 +197,27 @@
>
> void AsmStmt::EmitImpl(Serializer& S) const {
>   S.Emit(AsmLoc);
> +
>   getAsmString()->EmitImpl(S);

Directly emitting the StringLiteral object is okay if you believe that  
no object outside of the AsmStmt will EVER hold a reference to it.   
Otherwise, I would use EmitOwnedPtr.  This reasoning applies to all  
the places where StringLiteral appears in AsmStmt.  If you don't use  
EmitOwnedPtr (since I assume AsmStmt owns the references to these  
StringLiterals), the serializer won't be able to backpatch or  
otherwise fix up other references to these objects upon deserialization.

>
>   S.Emit(RParenLoc);

Eventually we will probably reorganize the order of how some of these  
fields are emitted because it will result in a smaller size on disk.   
For example, the SourceLocation objects and integers (e.g., the sizes)  
should probably be grouped together in the front, as the Serializer  
can group them nicely into a single record, while the StringLiteral  
object and the strings will likely terminate a record.  This isn't so  
important now; once we start adding support for abbreviations in the  
Serializer than we can play all sorts of tricks to reducing the size.   
AsmStmts are also not so common as other statements, so it isn't as  
big a deal to squeeze out every bit we can from the pickled format.   
The important issue now is that the implementation is correct, which  
it appears to be from looking at the code.   If you can continue to  
add tests cases into the clang test suite that tests the functionality  
of AsmStmt, then we will be able to readily test that this  
serialization code is correct using the --test-pickling in the driver.

<SNIP>

>
> AsmStmt* AsmStmt::CreateImpl(Deserializer& D) {
> @@ -206,7 +225,37 @@
>   StringLiteral *AsmStr = StringLiteral::CreateImpl(D);
>   SourceLocation PLoc = SourceLocation::ReadVal(D);

<SNIP>

>
> +  Stmt->Names.reserve(size);
> +  for (unsigned i = 0; i < size; ++i) {
> +    std::vector<char> data;

One recommendation is to move the vector "data" outside of the loop,  
and invoke "data.clear()" at the beginning of the loop body.  This  
will reduce the number of allocations made when the vector is resized.

Looks good!  I'm glad you could successfully de-obfuscate my  
completely undocumented serialization API.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20071121/456d06bd/attachment.html>