<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Anders,<div><br class="webkit-block-placeholder"></div><div>The serialization code looks good to me. A few comments (inline):</div><div><br><div><div>On Nov 21, 2007, at 5:36 PM, Anders Carlsson wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-size: 14px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0; ">Modified: cfe/trunk/AST/StmtSerialization.cpp<br>URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/AST/StmtSerialization.cpp?rev=44266&r1=44265&r2=44266&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/AST/StmtSerialization.cpp?rev=44266&r1=44265&r2=44266&view=diff</a><br><br>==============================================================================<br>--- cfe/trunk/AST/StmtSerialization.cpp (original)<br>+++ cfe/trunk/AST/StmtSerialization.cpp Wed Nov 21 19:36:19 2007<br>@@ -197,8 +197,27 @@<br><br>void AsmStmt::EmitImpl(Serializer& S) const {<br> S.Emit(AsmLoc);<br>+ <br> getAsmString()->EmitImpl(S);</span></blockquote><div><br class="webkit-block-placeholder"></div><div>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.</div><br><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-size: 14px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0; "><br> S.Emit(RParenLoc); </span></blockquote><div><br class="webkit-block-placeholder"></div><div>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.</div><div><br class="webkit-block-placeholder"></div><div><SNIP></div><div><br class="webkit-block-placeholder"></div><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-size: 14px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0; "><br>AsmStmt* AsmStmt::CreateImpl(Deserializer& D) {<br>@@ -206,7 +225,37 @@<br> StringLiteral *AsmStr = StringLiteral::CreateImpl(D);<br> SourceLocation PLoc = SourceLocation::ReadVal(D);</span></blockquote><div><br class="webkit-block-placeholder"></div><div><SNIP></div><br><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-size: 14px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0; "><br>+ Stmt->Names.reserve(size);<br>+ for (unsigned i = 0; i < size; ++i) {<br>+ std::vector<char> data;</span></blockquote><div><br class="webkit-block-placeholder"></div><div>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.</div></div><div><br class="webkit-block-placeholder"></div><div>Looks good! I'm glad you could successfully de-obfuscate my completely undocumented serialization API.</div></div></body></html>