[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

[Anders Carlsson]

21 nov 2007 kl. 23.16 skrev 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.

That should not be a problem. Eventually all the uses of StringLiteral  
should be replaced by regular std::strings.

>>
>>
>> +  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.

Good point, I'll do that.

>
> Looks good!  I'm glad you could successfully de-obfuscate my  
> completely undocumented serialization API.

I've seen much much worse APIs ;)

Anders
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20071121/2ba12f73/attachment.html>